fix managed thread factory tests - context is saved before execution, not jndi lookup

[aubi]

Hello,
the tests expect, that saving of the current thread context happen when the object (e.g. ManagedThreadFactory) is looked up in JNDI tree.
This seems strange -- it makes much more sense to save the context just before the asynchronous task is executed -- at the beginning of ManagedThreadFactory.newThread() or ForkJoinPool.submit().

I modified the tests to change the context of the launching thread after these functions. It shouldn't break an implementation doing context save on JNDI lookup, so existing implementation should still pass.

[njr-11]

the tests expect, that saving of the current thread context happen when the object (e.g. ManagedThreadFactory) is looked up in JNDI tree.
This seems strange -- it makes much more sense to save the context just before the asynchronous task is executed

We cannot change this one. Capturing the context upon ManagedThreadFactory lookup has actually been a requirement since the 1.0 version of the Concurrency spec (and ever since), where it states that the new thread "will run with the application component context of the component instance that created (looked-up) this ManagedThreadFactory instance". I can speculate that the reason for doing this is to provide a consistent behavior when ManagedThreadFactory is used as the thread factory for thread pools. By using the context at lookup, all threads in the pool run with the same context, rather than some requests leading to some pooled threads with one context and other pooled threads with another context, where a request that ends up with a pooled thread could get either.

[njr-11]

Here is some additional language from the specification document section on ManagedThreadFactory, consistent with what I stated above:
"When the thread is
started using the Thread.start() method, the Runnable that is executed
will run with the context of the application component instance that
created the ManagedThreadFactory instance.
NOTE
The ManagedThreadFactory instance may be invoked from several
threads in the application component, each with a different container
context (for example, user identity). By always applying the context of
the ManagedThreadFactory creator, each thread has a consistent context."

[smillidge]

I'm not too clear on what the test is trying to do. It seems to set some application context and then mutate the same context while expecting to retrieve the old value.

My simplistic reading is that you are expecting the ManagedThreadFactory to take a point in time immutable snapshot of the application context at lookup time. I think that is a stretch of the spec wording.

I will check with Petr.

[njr-11]

A post on the mailing list from @aubi suggests that the implementation in question might have a limitation against being able to create a new ManagedThreadFactory instance per lookup, and if that is the case would cause the specification requirement of applying the context of the ManagedThreadFactory creator to managed threads to be an undue burden if not entirely unachievable, which would be valid grounds for a TCK challenge under “Claims that an assertion of the specification is not sufficiently implementable.” If you are able to confirm that is the case, I could support a challenge on that basis.

[aubi]

@njr-11 The question is not, if the ManagedThreadFactory should be created every time it is looked up from JNDI. We don't do, but it isn't the key point.

The context mentioned in the spec (3.4.1, "All Threads are contextual...") is definitely propagated - it is the component, from which the factory is used. But it (unfortunately) doesn't say, WHEN it should be captured. To me, creation of a new thread seems as reasonable place.

As I understood in mail from Eduardo (https://www.eclipse.org/lists/cu-dev/msg00283.html), the thread always happened in newThread(), at least in Glassfish and WildFly.

I suggest to remove this requirement as both implementation make sense and it should be agreed and clarified, preferably in 3.1.

[njr-11]

The context mentioned in the spec (3.4.1, "All Threads are contextual...") is definitely propagated - it is the component, from which the factory is used. But it (unfortunately) doesn't say, WHEN it should be captured. To me, creation of a new thread seems as reasonable place.

The second part of that bullet under 3.4.1 elaborates on when. Here is the full text of that section,

• All Threads are contextual (see section 2.3). When the thread is started using the Thread.start() method, the Runnable that is executed will run with the context of the application component instance that created the ManagedThreadFactory instance.
  The ManagedThreadFactory instance may be invoked from several threads in the application component, each with a different container context (for example, user identity). By always applying the context of the ManagedThreadFactory creator, each thread has a consistent context. If a different context is required for each thread, use the ContextService to create a contextual object (see section_ 3.3).

Like all things, the text has its imperfections and could certainly be improved. For example, by inserting the text in bold italics below,

"By always applying the context of the ManagedThreadFactory creator as it was at the point in time when the creator looked up the ManagedThreadFactory, each thread has a consistent context."

However, even without that language I don't see how the interpretation of capturing context upon newThread can align with the last 2 sentences. If you capture context upon newThread, you do not end up with consistent context across threads as it states. When a ManagedThreadFactory like that is supplied to create a thread pool, tasks submitted to the thread pool run with non-deterministic user identity. Not every use of ManagedThreadFactory will be for a thread pool though, and the last sentence of the spec selection cited acknowledges that it is a valid pattern to want different context for each thread instead of consistent context across threads. It tells you how to achieve that pattern. The spec does not tell you to achieve it with standard use of ManagedThreadFactory, it states to use ContextService, which can individually contextualize the Runnable per thread, overriding what the ManagedThreadFactory normally gives you. Why explicitly state to use ContextService to achieve the behavior of different context per thread if the ManagedThreadFactory were already giving you that? This section reveals the intent of the spec alongside its requirements. The requirement is obscure though, buried within these sections, and it seems very likely that some implementations would have simply overlooked it, which would be very understandable.

Although I'd really like to help you out and get more implementations passing the TCK by overlooking this, I'm doing my best to honestly interpret the spec as written and can't give an interpretation that strays from my best understanding of it. That said, I don't think you need my agreement to move forward on the challenge. If others vote to accept the challenge, of if you use lazy consensus and no one else has a dissenting view (as is currently the case), my divergent understanding of it won't make any difference. You don't need unanimity to get a challenge accepted. And if for some reason that challenge fails, you could always come back and challenge it again on the basis of "not sufficiently implementable" which I would be able to support as indicated earlier.

[smillidge]

My main concern is that the implementation we are using was the original reference implementation and the additional test is breaking the reference implementation. Saying that there is no longer a reference implementation at Eclipse. The Payara team actually have a way of implementing the interpretation described by @njr-11 however we have concerns that this could break applications out in the wild. I think the pragmatic approach is to accept the challenge at this stage and then discuss for the next release what we think should be correct behaviour.

[OndroMih]

I thought about this for a while and basically agree with all views presented:

• I agree with @njr-11 that the spec suggests that the context is captured when the thread factory is created/injected/looked up. This is even more explicitly written in the Javadoc
• I understand @aubi's point that it's more natural that the context is captured when the thread is created and not when the factory is created/injected. For an average developer, it's more natural to pass the context that exists when a new thread is created and started rather than capture the context when the factory was created. For example, if some component receives a factory and uses it to create a new thread, a developer would most likely want to continue with the same context it has, and not with the context that existed when the factory was created. However, I'm not firm on this, there might be relevant usecases where this is not desired, I just can't think of any
• I also understand @smillidge's point that GlassFish used to serve as the reference implementation and back then, it was expected that all other implementations behave like GlassFish. However, in practice, this didn't always happen. And now I wouldn't take GlassFish as an example because other implementations may serve as CIs in the future and it could completely revert the argument.

My view is that the spec is more or less clear about it. As @njr-11 says, the challenged test is aligned with the spec and Javadoc. However, I think that the new features added in EE 10 match with this. I believe that the behavior described in the spec was defined a very long time ago, probabl wasn't properly covered by the TCK and possibly implementations didn't comply with it.

If we ensure that the behavior defined in the spec is covered by new TCK tests or change the behavior, either way I expect that some implementations will need to change their behavior that used to pass with older TCKs and thus potentially break applications. So I would approach this carefully.

If we don't want to break applications, we change what the spec says and make the test less strict, to allow the context capture either when factory is created or when new hread is created. This is I believe what @aubi suggested in the end.

If we want to specify a protable behavior, then I would question whether the behavior currently specified in the spec makes sense or should be changed. The fact that it was defined in the spec and not covered by tests is unfortunate. But it also allows us to change the behavior if we think it's better to change it.

[mswatosh]

For example, if some component receives a factory and uses it to create a new thread, a developer would most likely want to continue with the same context it has, and not with the context that existed when the factory was created. However, I'm not firm on this, there might be relevant usecases where this is not desired, I just can't think of any

My first thought for a relevant use case was if someone wanted each thread to be created with identical context, and then I saw Nathan pointed out a good example, if ManagedThreadFactory is used to back a thread pool:

I can speculate that the reason for doing this is to provide a consistent behavior when ManagedThreadFactory is used as the thread factory for thread pools. By using the context at lookup, all threads in the pool run with the same context, rather than some requests leading to some pooled threads with one context and other pooled threads with another context, where a request that ends up with a pooled thread could get either.

While I agree that it seems more intuitive to capture context at thread creation, it also seems like that could lead to confusion, if you're running the same task multiple times and context changes between thread creation.

[OndroMih]

While I agree that it seems more intuitive to capture context at thread creation, it also seems like that could lead to confusion, if you're running the same task multiple times and context changes between thread creation.

I see this as completely opposite :) If a dev creates a task, I think they would want to run it in the context of the current thread, even if the context changes. Think about security context. A task would change settings in the database and save the current user for auditing purposes. It should save the user who launched the task, not the one that created the task.

If it’s desired to run a task with a specific context captured at some point, a contextual proxy can be created. I strongly think this would be much less frequent.

[aubi]

I added tests with injected MES: #423
So the question is: if the thread context is captured when picked up from JNDI -- what is the supposed context? Beginning of a function?
The only obvious code related to a factory is:

Thread thread1 = threadFactoryInjA.newThread(() -> {
            });

[njr-11]

I added tests with injected MES: #423 So the question is: if the thread context is captured when picked up from JNDI -- what is the supposed context? Beginning of a function? The only obvious code related to a factory is:

Thread thread1 = threadFactoryInjA.newThread(() -> {
            });

Excellent- I'll try to find some time to review that pull.
To answer your question above, it looks like an instance of the managed thread factory is injected into threadFactoryInjA here:

public class ManagedThreadFactoryDefinitionInjectedServlet extends TestServlet {
    ...

    @Resource(lookup = "java:app/concurrent/ThreadFactoryInjA")
    private ManagedThreadFactory threadFactoryInjA;

which is where the JNDI lookup is performed in order to inject it, so it would capture the context that is on the thread during the initialization of that Servlet.

[OndroMih]

which is where the JNDI lookup is performed in order to inject it, so it would capture the context that is on the thread during the initialization of that Servlet.

I still don't get it why the context should be captured during injection and not when the executor is used. What's the context of a servlet being initialized? If the servlet is set to initialize on startup, for example? I really think that in most cases, developers would want to use the context of the thread that submits the tasks and not the context that exists when the executor is injected. In the rare case, when a developer needs the context at injection, they can inject the executor into a constructor and then wrap it in a contextual proxy to keep the current context.

[njr-11]

I still don't get it why the context should be captured during injection and not when the executor is used. What's the context of a servlet being initialized? If the servlet is set to initialize on startup, for example? I really think that in most cases, developers would want to use the context of the thread that submits the tasks and not the context that exists when the executor is injected. In the rare case, when a developer needs the context at injection, they can inject the executor into a constructor and then wrap it in a contextual proxy to keep the current context.

It sounds like you are confusing executors and thread factories here. The question from @aubi was about ManagedThreadFactory. Your comment was about executors. I agree with what you said about executor, and the Concurrency spec aligns with that model as well. For ManagedExecutorService and ManagedScheduledExecutorService, the spec has the context being captured at submit time. But for ManagedThreadFactory, the Concurrency spec defines that context capture happens when the managed thread factory is created or looked up. Its reason for doing this is to allow the ManagedThreadFactory instance to back a custom thread pool that obtains its threads from a java.util.concurrent.ThreadFactory, such as the ones that are conveniently obtainable that way from from java.util.concurrent.Executors and have deterministic thread context on every thread in the pool, rather than an unpredictable behavior of the contexts of which ever requests happened to trigger newThread. Whether that pattern is more prevalent than direct use of newThread or not, I cannot say. I can say that over the years as customers have come to us wanting to use their own custom thread pools and get context propagation for them, we have pointed them to this ability that is offered by the Concurrency specification. We will not be able to change the behavior without breaking all of those customers. That is true even if their usage pattern is in the minority.

[OndroMih]

Yes, you’re right, I confused executors and factories. I still think that it would be more intuitive and in line with executors to capture the context when newThread is called. It would be also easier to implement because the the context would be captured by the injected factory, while the behavior you support requires implementations to capture the context in all the in ways in which a factory can be retrieved (JNDI, Injection,…). I think that this behavior has never been clearly defined in the spec, it’s also not supported in GlassFish, which used to be the reference implementation. So I would say that if your implementations behave like you suggest, it’s rather an implementation detail, not something set in stone in the spec. If we define the behavior as I suggest, it should be easy to providea compatibility switch in your implementation for your customers.

I’d like to see opinions from other people, from other implementors, before we cement this in the TCK. If others support the behavior you suggest, I’d happily accept it.

[njr-11]

I’d like to see opinions from other people, from other implementors, before we cement this in the TCK. If others support the behavior you suggest, I’d happily accept it.

I would like to see that as well. I was surprised that no other vendors chimed in during the initial TCK challenge last release nor have they since stated a position on the matter.

The specification requirement is pretty clearly defined in several places.

Even the usage scenario of supplying ManagedThreadFactory to custom thread pools is stated in the introductory paragraph (which is only 2 sentences long) on ManagedThreadFactory:

3.4. ManagedThreadFactory

The jakarta.enterprise.concurrent.ManagedThreadFactory allows applications to create thread instances from a Jakarta EE Product Provider without creating new java.lang.Thread instances directly. This object allows Application Component Providers to use custom executors such as the java.util.concurrent.ThreadPoolExecutor when advanced, specialized execution patterns are required.

That scenario only works if the context is deterministic and captured at a single point, as the spec defines here:

• All Threads are contextual (see section 2.3). When the thread is started using the Thread.start() method, the Runnable that is executed will run with the context of the application component instance that created the ManagedThreadFactory instance.
  | The ManagedThreadFactory instance may be invoked from several threads in the application component, each with a different container context (for example, user identity). By always applying the context of the ManagedThreadFactory creator, each thread has a consistent context. If a different context is required for each thread, use the ContextService to create a contextual object (see section 3.3).

ManagedThreadFactory JavaDoc is consistent with the spec,

The Runnable task that is allocated to the new thread using the ThreadFactory.newThread(Runnable) method will run with the application component context of the component instance that created (looked-up) this ManagedThreadFactory instance.

There is also some newer language in the spec/javadoc saying the same thing, but the above have been there from version 1.0. I really don't see how this can be interpreted any other way. It's unfortunate that the scenario was missed from the TCK which allowed Glassfish to overlook the requirements and implement a different behavior. Both behaviors are useful, but only one is stated in the spec and JavaDoc as the required behavior, which is what has the final say here.

It end up that the best resolution to this will be to add a switchable setting to the spec to let the user control which behavior they want for a ManagedThreadFactory, whether that happens now or in a future release. I'm interested to hear back what all of the other vendors have done in their implementations.

[OndroMih]

Thank you, @njr-11 , for explanation and citations from the spec. You're right, the spec suggests that all threads created by a factory have the same context by default. However, it's not clear what's the context. The spec says that threads should have "context of the application component instance that created the ManagedThreadFactory instance". However, I don't see where it defines what is the application component instance that created the factory. It says that factory instances can be retrieved via JNDI or injection:

"ManagedThreadFactory instances are retrieved using the Java Naming and Directory Interface (JNDI) Naming Context (EE.5) or through injection of resource environment references (EE.5.8.1.1)."

But it doesn't clarify how the factories are created. It's up to the "Jakarta EE Product Providers" (implementations) to somehow create factories as managed objects. In one way, we can interpret it that the context should be already set when a factory is retrieved (via JNDI or injection) and then never changed. But it doesn't have to be the context of the component that retrieves the factory, it can be a context of any component in an app server, which is very vague. For example, if a factory is defined in GlassFish globally for the whole server, it's created once and shared by every application. Then it's hard to say what's the context in that case, if the factory is created before an application even exists.

So, I sort of get it why the spec suggests that the context shouldn't change for threads created by a factory, but it should be also defined when and which context is captured. And I don't see how this could be reasonably specified. That's why I don't like the behavior explained in the spec, as it makes some assumptions which don't apply to all Jakarta EE implementations.

[OndroMih]

Only this part of the specification suggests that the context should be the context of the application component that retrieved the factory:

All Threads are contextual (see section 2.3). When the thread is started using the Thread.start() method, the Runnable that is executed will run with the context of the application component instance that created the ManagedThreadFactory instance.

However, it's the only place which says about an application component instance "creating" a factory instance. All other places, several times in the spec, say that application component instance "retrieves" a factory instance (which I understand that they retrieve an instance created by an app server). This is very confusing - which is then the component that "creates" the factory - an app server component, or an application component (e.g. EJB)? If it's application component as @njr-11 suggests, what is then the context of the component? I always thought that a context is associated to a thread that executes a component. If an application component is created as a CDI session bean, does it have the context of the thread that created the bean (i.e. the thread that started a session)? If that's the case, then the context is not deterministic, and I would rather prefer if the context is captured from the thread that calls the newThread method, or inside a contextual proxy, which a developer creates with the context they want to capture.

[aubi]

I still see a big misunderstanding, what "context" means. I think, that the spec talks about JNDI context! Therefor it influences, how "java:comp/..." or "java:app/..." are evaluated. The spec makes clear sense -- the component, which creates the factory or picks it from JNDI.

The big difference came in 3.0 with the way, how data can be moved among threads, e.g. it became important also "thread context" and the moment, when it was captured. And I think the old text doesn't talk about this situation at all.

Also, loading factory from JNDI is one way and there can be the moment, when to capture thread context as @njr-11 suggests. The other situation, when the factory is injected, is then completely out of my imagination, how it should behave. What thread should be used to capture the thread context?

[njr-11]

I agree a lot of the language in the spec could certainly use improvement in this area. However, it doesn't look like there will be agreement on what to change it to. I don't see this getting resolved without consuming a lot of time and effort. Given the timing of Jakarta EE 11, I suggest deferring the discussion to the release of Concurrency spec for EE 12, where we can hopefully work on this earlier in the release cycle to ensure a good resolution. I believe the TCK tests for it were disabled during EE 10 due to a controversial challenge. We should be able to leave them that way for EE 11, and implementations will pass with either behavior as they did in EE 10/Concurrency 3.0

[aubi]

@njr-11 I agree. I didn't prepare the samples for discussion soon enough :-(
Let's keep the current status and return to this discussion later.