better definition of application context

[njr-11]

fixes #192

[manovotn]

Looking at TCKs, we only have this test. We should revisit it too because:

• the test is currently optional only which I am not sure is needed/intentional
• the test only verifies that there is some TCCL present, not that it is the same one; e.g. I am not sure the test is actually testing anything (or am I missing something?)

[njr-11]

Looking at TCKs, we only have this test. We should revisit it too because:

* the test is currently optional only which I am not sure is needed/intentional

* the test only verifies that there is _some_ TCCL present, not that it is the same one; e.g. I am not sure the test is actually testing anything (or am I missing something?)

I'd be happy to switch the test to require application context. Beyond that, testing for a JNDI namespace would require that we add a Jakarta/Java EE resource reference to the TCK application, which currently doesn't depend on Jakarta/Java EE. Is that a concern here? As for the thread context class loader - it is at least being used to load an application class. But I could see some implementations getting around that by always having all classes available.

[FroMage]

Huh, I never understood what application-scoped meant, which is why we don't have an application-scoped context provider in Quarkus. I guess there's value in providing propagation for TCCL (we've had users ask for that), but I wonder what will happen if we make this the default (could be some stuff breaks, but I hope not). We don't support JNDI though, so we should not request the application context to propagate JNDI even if the platform doesn't support JNDI.

TBH I find these predefined contexts just don't match with the real world outside of EE, because in Quarkus we have some request objects in the CDI context, others in the RESTEasy context and yet others in the Servlet context. Our security is using CDI, so we have no Security context (it's already in the CDI one), and the TCCL is actually a constant since we can only have a single application, so capturing/restoring it doesn't make a lot of sense in most situations where the framework manages the threads, because we can simply make sure to always set it for those threads, without MP-CP being involved. This is less true if the threads are not managed by the framework.

So, I'm fine with asking that the Application context propagates the TCCL (we do have to change the test, it currently only checks that the thread can load the same classes as the test itself, and I suspect it's only passing in Quarkus because our thread pool has the TCCL set, not by MP-CP). But I'd don't want to specify that the Application must propagate JNDI if it's not available.

[manovotn]

TBH I find these predefined contexts just don't match with the real world outside of EE, because in Quarkus we have some request objects in the CDI context, others in the RESTEasy context and yet others in the Servlet context. Our security is using CDI, so we have no Security context (it's already in the CDI one), and the TCCL is actually a constant since we can only have a single application, so capturing/restoring it doesn't make a lot of sense in most situations where the framework manages the threads, because we can simply make sure to always set it for those threads, without MP-CP being involved.

This nicely sums up that what context propagation means can easily differ between platforms using different implementations. And the harder we try to pre-define it and set defaults, the less realistic it can become for wider use (in between several platforms).

So, I'm fine with asking that the Application context propagates the TCCL (we do have to change the test, it currently only checks that the thread can load the same classes as the test itself, and I suspect it's only passing in Quarkus because our thread pool has the TCCL set, not by MP-CP). But I'd don't want to specify that the Application must propagate JNDI if it's not available.

+1

[njr-11]

@manovotn I added a commit to update the test case per your review comments. Thanks for spotting that!

[FroMage]

So, the question came up of whether it's safe/desirable to set the TCCL to null in ThreadContextProvider.clearedContext? WDYT? ATM the JDK sets the TCCL for every new thread, so this could be dangerous, but I'm not sure how we can obtain that default TCCL if that's what we want for the "cleared" semantics.

[manovotn]

Looking at Stephane's PR for SR, I was thinking - what should happen when you say you want application context cleared? Namely I am looking at TCCL.

First thing that comes to mind is to just set it to null. However, I think that is very rarely useful and can even be harmful in some cases. Shouldn't we specify that we just "ignore" it in that case?

[FroMage]

Javadoc says this about the initial TCCL:

Returns the context ClassLoader for this thread. The context ClassLoader is provided by the creator of the thread for use by code running in this thread when loading classes and resources. If not set, the default is the ClassLoader context of the parent thread. The context ClassLoader of the primordial thread is typically set to the class loader used to load the application.

So it's not necessarily trivial to obtain the right initial TCCL to restore it to, but null ain't it.

[njr-11]

I checked on what the OpenLiberty implementation does when clearing application context, and it sets the system classloader, ClassLoader.getSystemClassLoader(). This same code is also used in our implementation of Java EE Concurrency, which has been around for years, and we have yet to see anyone raise a complaint over it. My guess is that this choice was match to match the behavior of setting a null thread context class loader. As specified by java.lang.Thread, null indicates the system class loader (or, failing that, the bootstrap class loader).

Given that managed executors allow for the use of pooled threads, some of which might be created lazily, possibly with different class loaders on the parent thread at the time, and we want deterministic usage in all cases, there isn't really a proper default TCCL to clear to. Ignoring rather than clearing the thread context class loader seems like a bug to me, because if a user specified they want it cleared, it could mean they don't want whatever code they are submitting/scheduling/invoking to be able to load their application classes.

What are the cases where you see a cleared thread context class loader being harmful? And why wouldn't the user just specify to propagate or ignore in those cases?

[manovotn]

I have seen some cases (bugs) on WFLY where they were relying on TCCL never being null and that failed for instance in case of async events with Weld (where we provide executor with nulled TCCL intentionally). So you might be right that it is indeed correct because I found the same javadoc notion of null being a valid value meaning basically "system CL".