So this is somewhat of a topic larger than one PR. First some background on architecture. The ThreadContext is essentially an API wrapped around a ThreadLocal, designed to be the only thread local you actually need to track thread state.
In many other app servers some of us had worked on in the past, there tended to be a lot of ThreadLocals. A big issue was state not getting cleaned up, resulting in it bleeding into other requests and other forms of leaks. The unbreakable golden rule of our code base is ThreadContext.enter() and ThreadContext.exit() must be called in the same method, surrounded by a try/finally. Enter before the try, and exit in the finally. The second golden rule is a ThreadContext must be only associated with one thread at a time. The ThreadContextListener API is how we populate and propagate thread state. These should be thread-safe. Any objects copied from one ThreadContext to another should be thread-safe. Now some strangeness I noticed in our code. RequestScopedThreadContextListener was turned into a static field in CdiBuilder. This is needless as the list of listeners in ThreadContext are already static. Also, adding the same listener instance multiple times does nothing useful and creates a brief write lock on CopyOnWriteArraySet -- harmless, but also needless. It causes no harm but is a code smell. ThreadContextListener.contextEntered was changed to have a boolean propagateTx argument. We should remove that as a parameter and make it an entry in the ThreadContext with a corresponding ThreadContextListener that adds it and propagates it and ideally facilitates the transaction propagation. The default cannot be "true" as in EJB all asynchronous calls should NOT propagate transaction and security. Propagation should be limited to the spec that requires it, which I think is Jakarta Concurrency scenarios. Historically security and transaction state was not propagated to other threads in scenarios like async calls, because those things were inherently not thread safe. The Geronimo TransactionManager is not capable of supporting a transaction across multiple threads. To my knowledge it uses a ThreadLocal itself to track state and related code such as UserTransaction, TransactionSynchronizationRegistry and JTA as a spec are not capable for coordinating one transaction across multiple threads, nor are specs on top such as JPA. We may need to review all the recent code that propagates state from one thread to another for async calls. In most cases, the instances we propagate from one thread to another will likely not be thread-safe and need a major overhaul or complete rewrite unless they are fully read-only and immutable. If you've added some code that propagates state in the last couple years, can you follow up with some details? I'm happy to take a look and we can talk it through. Double check it's safe. -David
