| Re: [cu-dev] Correct context save for ManagedThreadFactory |
|
For 1, JNDI lookup that is peformed by resource injection of a ManagedThreadFactory, I can see where it’s a bit unclear about the thread performing the lookup/creation of the ManagedThreadFactory. It would be a good idea for the spec to
elaborate more on its requirements for this scenario. I searched the TCK to identify whether it is obtaining a ManagedThreadFactory by resource injection anywhere. None of the newly added test are doing this, so it is not a cause of the errors you are running
into. I did find it being done in 2 of the pre-existing TCK tests from Concurrency 2.0 (I checked the old platform TCK to be sure a change to use resource injection wasn’t introduced during porting – it wasn’t). Neither of these make any assertions about
context, @Local(ContextPropagateInterface.class) @Stateless public class ContextPropagateBean implements ContextPropagateInterface { @Resource(lookup = "java:comp/DefaultManagedThreadFactory") private ManagedThreadFactory threadFactory; and @WebServlet("/testServlet") public class TestServlet extends HttpServlet { @Resource(lookup = "java:comp/DefaultManagedThreadFactory") private ManagedThreadFactory factory; (The latter is renamed form TestServlet to APIServlet after the port if anyone is looking for the equivalent in the 3.0 TCK) The specification document, going back to 1.0, also contains 2 examples with resource injection of ManagedThreadFactory, @Resource(name="concurrent/ThreadFactory") ManagedThreadFactory threadFactory; @PostConstruct public void postConstruct() { threadPoolExecutor = new ThreadPoolExecutor( 5, 10, 5, TimeUnit.SECONDS, new ArrayBlockingQueue<Runnable>(10), threadFactory); } and @Resource(name=”concurrent/LoggerThreadFactory”) ManagedThreadFactory threadFactory; I can tell you how Open Liberty attempted to best follow the intent of the spec in this case, which is that we capture the identity of the application component into which the resource is being injected and apply that to threads created
by the ManagedThreadFactory in order to allow the JNDI lookups on its application component namespace, and we would be using empty context for most other types because there is nothing much else on the thread performing the initialization to capture context
from. That was our interpretation of intent – I’m not claiming that the spec provides sufficient detail on the resource injection path to require that much of other implementations. We could open an issue, copying from the above information for Concurrency vNext to clarify context requirements for the resource injection path. Or we could leave it vendor-specific. Either would be fine with me, but it doesn’t seem
an immediate concern because existing tests would already be passing and the new tests don’t use this pattern. For 2, when multiple threads perform, ManagedThreadFactory mtf = InitialContext.doLookup("java:app/concurrent/mtf1"); the specification does not have a requirement that the same instance be returned from each lookup. The Open Liberty implementation chose to return a different instance for each ManagedThreadFactory lookup, which we did specifically to
attach the context information to enable our own way of meeting the specification requirement of applying the context of the ManagedThreadFactory creator to the managed threads. I didn’t realize other implementations would have limitations against using multiple
instances. For implementations that must return the same instance from lookups, it seems like this specification requirement would cause a lot of difficulty, risk, and possibly be unimplementable. If you are going to file a challenge to the TCK, I think
that is the grounds to do it upon. To use the exact words from Scott’s email: “Claims that an assertion of the specification is not sufficiently implementable.” As far as usage patterns go, the spec is currently optimized toward a ManagedThreadFactory being used to back a thread pool. In that case, you want the predictability of every thread running with the same context, regardless of whether
you happened to get a thread that was created in response to your current request or to some other previous request. It’s also highly efficient because there is little overhead to capturing, establishing, and removing context from threads – the context just
stays on the pooled threads. The obvious disadvantage is that the container isn’t managing the thread pool. But the pattern you are thinking about with direct usage of newThread makes sense as well. An idea for a future spec version could be to allow either and make the behavior configurable on ManagedThreadFactoryDefinition, but that probably
only works for you if your implementation would have a way of supporting both behaviors. We can leave that idea to future discussion and open an issue for it if it is of any interest. From: cu-dev <cu-dev-bounces@xxxxxxxxxxx> on behalf of Petr Aubrecht <aubrecht@xxxxxxxxxxxx> Hello everybody, I have an issue, what is the expected behavior of ManagedThreadFactory, particularly when the calling thread's context should be saved; my expectation
was that it is similar to ManagedExecutorService. I did PR and got answer ZjQcmQRYFpfptBannerStart
ZjQcmQRYFpfptBannerEnd Hello everybody, I have an issue, what is the expected behavior of ManagedThreadFactory, particularly when the calling thread's context should be saved; my expectation was that it is similar to ManagedExecutorService. I did PR and got answer
from Nathan, that it should happen on JNDI lookup(): https://github.com/jakartaee/concurrency/pull/212 The javadoc for ManagedThreadFactorysays this:
https://github.com/jakartaee/concurrency/blob/1acd0022c9955f2a6c7da5604ac9b0e09712fd31/api/src/main/java/jakarta/enterprise/concurrent/ManagedThreadFactory.java#L40 * {@link ThreadFactory#newThread(Runnable)} method * will *run with the **application component
context of the component instance* * that created (looked-up) this ManagedThreadFactory instance. * The Runnable task that is allocated to the new thread using the I understand it this way -- when executed, the java:comp is set to the EJB doing the JNDI lookup. But saving thread context at this moment is strange to me. Looking up in JNDI simply returns
the object, not initializing it. I have two questions, when it is expected to save the calling thread: 1) @Stateless class A { @Resource("java:app/concurrent/MTF1") private ManagedThreadFactory mtf1; public void f() { mtf1.newThread(() -> {}).start();
} } Here, the mtf1 field is initiated during creation of the EJB object A, which is definitely wrong thread for context save. 2) @Stateless class B { public void f1() { ManagedThreadFactory mtf = InitialContext.doLookup("java:app/concurrent/mtf1");
mtf.newThread(() -> {}).start(); } public void f2() { ManagedThreadFactory mtf = InitialContext.doLookup("java:app/concurrent/mtf1"); mtf.newThread(() -> {}).start(); } public void f3() { ManagedThreadFactory
mtf = InitialContext.doLookup("java:app/concurrent/mtf1"); mtf.newThread(() -> {}).start(); } } All 3 methods return the same object. If they have to store the context after doLookup(), they would need to store it in ThreadLocal - correct? Also,
the factories must be notified, that they were looked-up, right? The easiest explanation seems to me to be most obvious behavior -- save the context during newThread and stored in the ManagedThread. It is consistent across all usages (lookup, even in multiple
invocations, injection) and also consistent with ManagedExecutorService. The same is with ForkJoinPool using ManagedThreadFactory, but let's solve ManagedThreadFactory itself first. Thank you for your opinion or explanation, where I'm doing a mistake. Petr
|