TOMEE-4516 - Threadsafe RequestScopedThreadContextListener

[ammannmi]

Storing the DestroyContext in the ThreadContext itself is not threadsafe if the context is propagated to another thread (this is done in the ApplicationThreadContextProvider). This can occure in ManagedExecutors. Dependant on the timing the main thread could remove the RequestScope unfortunally.

[jungm]

https://ci-builds.apache.org/job/Tomee/view/tomee-10.x/job/pull-request-manual-with-tck/23/

[otbutz]

Possibly related: https://issues.apache.org/jira/browse/TOMEE-4485

[rzo1]

Can we try to come up with a test case for this? Might be difficult to hit because of timing, but maybe worth a try?

[ammannmi]

The problem mentioned in https://issues.apache.org/jira/browse/TOMEE-4485 with the missing ClassLoader was the primary reason for the analysis. When ManagedExecutors where used for some asynchronous tasks, the error occured in the main (request) thread, dependant on the duration of the asynchronous tasks and the remaining time in the main thread.

[dblevins]

Hey All,

First some insights on the design of ThreadContext and TheadContextListener. It was essentially created to be the "one thread local to rule them all" and eliminate the need for multiple thread locals. A ThreadContext is supposed to be thread-safe and not shared by multiple threads. On the surface with that design in mind, there should be no reason to have a ThreadLocal in a ThreadContextListener as is done on this PR. That's essentially putting a ThreadLocal on a ThreadLocal. If handling of the first thread local is broken and the ThreadContext is getting leaked, we'd want to fix that issue as it would be very serious. Said another way, we'd want to fix the first TheadLocal handling vs leaving it broken and adding more TheadLocals.

@ammannmi @otbutz could you both subscribe to the dev list so we can talk this through there? Subscribe. I'll wait till tomorrow before starting a thread there so you have time to subscribe.

[rzo1]

Hey All,

First some insights on the design of ThreadContext and TheadContextListener. It was essentially created to be the "one thread local to rule them all" and eliminate the need for multiple thread locals. A ThreadContext is supposed to be thread-safe and not shared by multiple threads. On the surface with that design in mind, there should be no reason to have a ThreadLocal in a ThreadContextListener as is done on this PR. That's essentially putting a ThreadLocal on a ThreadLocal. If handling of the first thread local is broken and the ThreadContext is getting leaked, we'd want to fix that issue as it would be very serious. Said another way, we'd want to fix the first TheadLocal handling vs leaving it broken and adding more TheadLocals.

@ammannmi @otbutz could you both subscribe to the dev list so we can talk this through there? Subscribe. I'll wait till tomorrow before starting a thread there so you have time to subscribe.

Thanks for the details regarding the design :) - I've reverted the commit on main for now, so we can discuss the thing on the dev@ list in more detail.

[jungm]

quick side note, the commit that caused all this is fb6340d

I'm not exactly sure anymore why i chose to do it this way, but removing it now makes the concurrency TCK fail:

[ERROR] Failures: 
[ERROR]   TransactionTests>Arquillian.run:137->testCancelTransactionWithManagedScheduledExecutorService:89->TestClient.runTest:70->TestClient.assertSuccessfulURLResponse:143->ArquillianTests.assertTrue:108 Output did not contain successful message: SUCCESS expected [true] but found [false]
[ERROR]   TransactionTests>Arquillian.run:137->testRollbackTransactionWithManagedScheduledExecutorService:72->TestClient.runTest:70->TestClient.assertSuccessfulURLResponse:143->ArquillianTests.assertTrue:108 Output did not contain successful message: SUCCESS expected [true] but found [false]
[INFO] 
[ERROR] Tests run: 147, Failures: 2, Errors: 0, Skipped: 1

Might need to look at this again and figure out another way to pass these tests

[otbutz]

@ammannmi @otbutz could you both subscribe to the dev list so we can talk this through there? Subscribe. I'll wait till tomorrow before starting a thread there so you have time to subscribe.

done

[ammannmi]

I agree with @rzo1, the ThreadContext should be bound to one thread and not shared between threads because this would require, that the whole context is threadsafe. So the main reason for the problems is sharing the ThreadContext. When the context is needed, then it should be cloned - if the content of the context is cloneable. The solution in the pull request was to fix the symptom because I am missing the insight in the main concepts.

[ammannmi]

I subscribed to the list, I hope I did it correctly

[rzo1]

The thread is here https://lists.apache.org/thread/9hhxsrclzhc49ln74xc029j836d6g7zq