Pass MDC entries down to Executrix child threads

[drivenflywheel]

Supports additional context capture in log entries

[sambish5]

The public modifier can be removed, but otherwise lgtm

[jdcove2]

Two observations...

• "implements Runnable" should be used instead of "extends Thread" unless one is modifying the actual Thread behavior (i.e. overriding start(), interrupt(), etc.).
• The context is being set by one thread and gotten by another thread without synchronization, which can cause unpredictable results.

[drivenflywheel]

Two observations...

• "implements Runnable" should be used instead of "extends Thread" unless one is modifying the actual Thread behavior (i.e. overriding start(), interrupt(), etc.).

Noted, but not at all within scope of this PR. This comment at the top of ProcessReader.java suggest that that decision is already old enough to legally tie one on:

/*
 * ProcessReader.java
 *
 * Created on November 19, 2001, 5:28 PM
 */

🍺

• The context is being set by one thread and gotten by another thread without synchronization, which can cause unpredictable results.

The parent thread 1) creates the child, 2) explicitly passes a point-in-time copy of the context, 3) starts the child, and then 4) uses join() to block until the child is complete. The MDC is backed by a ThreadLocal map. What's there to synchronize?

[jdcove2]

"What is there to synchronize?"

The setting of the contextMap attribute of ProcessReader is done by the thread executing the Executrix.execute(...) method (Executrix:835-836), which should be the mobile agent thread. The reading of the contextMap attribute is done by the ProcessReader thread (ProcessReader:39). Therefore, two threads are looking at the same variable. Java threading has many "gotchas" when accessing variables from multiple threads like "atomic access" and "memory consistency errors". This situation falls more into the "memory consistency error" which is defined as "different threads having inconsistent views of what should be the same data". There are a number of ways to solve these types of "gotchas"; however, the simplest in this situation is to wrap all accesses to contextMap with a synchronized block in order to "guarantee that changes to the state of the object are visible to all threads." One place this is described is in the Java Tutorial on Concurrency ( https://docs.oracle.com/javase/tutorial/essential/concurrency/index.html ). The fact that the actual MDC object is thread local, there exists a point-in-time object that is a copy of the MDC objects' state and that the mobile agent thread has "joined" the ProcessReader thread have no bearing on this issue.

[drivenflywheel]

"What is there to synchronize?"

The setting of the contextMap attribute of ProcessReader is done by the thread executing the Executrix.execute(...) method (Executrix:835-836), which should be the mobile agent thread. The reading of the contextMap attribute is done by the ProcessReader thread (ProcessReader:39). Therefore, two threads are looking at the same variable. Java threading has many "gotchas" when accessing variables from multiple threads like "atomic access" and "memory consistency errors". This situation falls more into the "memory consistency error" which is defined as "different threads having inconsistent views of what should be the same data". There are a number of ways to solve these types of "gotchas"; however, the simplest in this situation is to wrap all accesses to contextMap with a synchronized block in order to "guarantee that changes to the state of the object are visible to all threads." One place this is described is in the Java Tutorial on Concurrency ( https://docs.oracle.com/javase/tutorial/essential/concurrency/index.html ). The fact that the actual MDC object is thread local, there exists a point-in-time object that is a copy of the MDC objects' state and that the mobile agent thread has "joined" the ProcessReader thread have no bearing on this issue.

Recapping: you're worried that something might update a ProcessReader's private contextMap in between these consecutive statements, which are executed on the only thread that has access to the two ProcessReader instances since those instances are method-local variables :

            stdOutThread.setContextMap(MDC.getCopyOfContextMap());
            stdErrThread.setContextMap(MDC.getCopyOfContextMap());

            stdOutThread.start();
            stdErrThread.start();

There aren't two threads looking at the same data: MDC.getCopyOfContextMap() returns a COPY of the MobileAgent thread values, and we directly assign that copy to the ProcessReader's private contextMap via the setContextMap(..) map. The only thing that could alter the ProcessReader's private contextMap would be another call by Executrix to setContextMap(..) before the first statement of the ProcessReader run() method executes.

If instead you're worried that the MDC values on the MobileAgent thread could change before the ProcessReader's run method executes, that's not a problem we care about. The copied values accurately reflect the serviceLocation and shortName for which the Executrix data was prepped, and that's the entire goal of this PR.

[jdcove2]

This discussion does not seem to be progressing as we seem to be talking past each other. Given the purpose of this PR, I am ok with the original code changes.

[drivenflywheel]

Popped off the last commit, now back to the previous state.