-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing Tests of Context Switch from Asynchronous Tests #497
Removing Tests of Context Switch from Asynchronous Tests #497
Conversation
I removed tests of context switch, which tested, that the context capture happened during jndi lookup. The test later setup number 22 and the test checks, if asynchronous method (running via scheduler, but only once) returns 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. This test checks just the context switch, not asynch methods running concurrently as it says in the message. Although it is called schedule, it runs only once.
This is not true. Notice that the name of the test method is "testScheduledAsynchIgnoresMaxAsync" indicating that it is testing that a Scheduled Asynchronous method runs even if the max async is used up by other operations. By removing the line to request the asynchronous method and removing the assertion of its completion, the test becomes useless and no longer tests what it was written for.
The discussion, if the context capture should happen on JNDI lookup or when the thread is created, is still open: #253
I don't see discussion of that issue under #253 . In any case, the discussion/debate is about context capture for ManagedThreadFactory, not ManagedExecutorService and ManagedScheduledExecutorService. The former is documented with a different behavior for context propagation than the latter. I am not aware of issues over the latter, but in any case it makes no difference whatsoever to this pull because the executors in question are not even configured to propagate the IntContext.
The executor here is java:module/concurrent/ExecutorB
:
@Asynchronous(executor = "java:module/concurrent/ExecutorB", runAt = @Schedule(cron = "*/3 * * * * *"))
public CompletableFuture<Integer> scheduledEvery3Seconds(final int runs, final AtomicInteger counter) {
Its configuration shows it to be using the java:module/concurrent/ContextB
ContextService.
@ManagedExecutorDefinition(name = "java:module/concurrent/ExecutorB", context = "java:module/concurrent/ContextB", maxAsync = 1)
java:module/concurrent/ContextB
is actually defined differently in 2 different modules, elsewhere making for a very good test of java:module
configuration.
In ContextServiceDefinitionBean
,
@ContextServiceDefinition(name = "java:module/concurrent/ContextB", propagated = { APPLICATION,
StringContext.NAME }, cleared = IntContext.NAME, unchanged = TRANSACTION)
In ContextServiceDefinitionServlet
,
@ContextServiceDefinition(name = "java:module/concurrent/ContextB", cleared = TRANSACTION, unchanged = { APPLICATION,
IntContext.NAME }, propagated = ALL_REMAINING)
In neither case is IntContext
propagated. In the EJB module, it is cleared. In the web module, it is left unchanged. The real problem here appears to be that the author of the test was likely looking at the wrong one (the EJB module) when writing the assertion. If it were supposed to be cleared, the assertion of 0 would be correct. However, the configuration that applies is the one for the web module where IntContext
must be left unchanged on the thread of execution. I would guess that most implementations would be running the method on a pooled thread that likely has has no IntContext
and inadvertently end up with 0 allowing it to pass. However, there is no requirement of that and so the assertion for a value of 0 is wrong.
In order to correct this in a way that preserves the intent/usefulness of the test case, I am recommending that the assertions be switched to assertNotNull so that it asserts completion of the scheduled asynchronous method. The return value does not matter.
The suggestions that I added do not include adding an import statement for assertNotNull. I could not see way for git to allow me to suggest that, so it would need to be added separately.
...tck/concurrent/spec/ManagedExecutorService/resourcedef/ManagedExecutorDefinitionServlet.java
Show resolved
Hide resolved
...c/ManagedScheduledExecutorService/resourcedef/ManagedScheduledExecutorDefinitionServlet.java
Show resolved
Hide resolved
...c/ManagedScheduledExecutorService/resourcedef/ManagedScheduledExecutorDefinitionServlet.java
Show resolved
Hide resolved
I see your point, Nathan. Your suggestions look good, I'll send a commit in a second. |
@njr-11 If you will agree with the changes, I'll merge the commits, so there is just one commit, not two doing remove/readd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@njr-11 If you will agree with the changes, I'll merge the commits, so there is just one commit, not two doing remove/readd.
It looks good now. Thanks for spotting that the tests had an issue here!
I removed tests of context switch, which tested, that the context capture happened during jndi lookup. The test later setup number 22 and the test checks, if asynchronous method (running via scheduler) returns 0.
The code:
reqBean.scheduledEvery3Seconds
runs only once and returnsIntContext
:future.complete(IntContext.get())
The discussion, if the context capture should happen on JNDI lookup or when the thread is created, is still open: #253