Propagate JNDI context if none exists. Fixes that tasks submitted to executor during deployment are executed #24544
Conversation
Passes Concurrency TCK in appserver/tests/tck/concurrency. Passes tests in appserver/tests/admin and application. Signed-off-by:Ondro Mihalyi <[email protected]>
Passes Concurrency TCK in appserver/tests/tck/concurrency. Passes tests in appserver/tests/admin and application. Signed-off-by:Ondro Mihalyi <[email protected]>
...concurrent-impl/src/main/java/org/glassfish/concurrent/runtime/ContextSetupProviderImpl.java
Show resolved
Hide resolved
|
I see that some EJB tests failed. I’ll investigate why. |
|
@OndroMih, build successful. |
Awesome 🙂 |
.../java/org/glassfish/main/test/app/concurrency/executor/ManagedExecutorDefinitionServlet.java
Outdated
Show resolved
Hide resolved
...n/tests/src/test/java/org/glassfish/main/admin/test/progress/ProgressStatusComplexITest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
|
|
||
| /** | ||
| * Check whether the application component submitting the task is still running. | ||
| * Throw IllegalStateException if not. | ||
| */ | ||
| private boolean isApplicationEnabled(String appId) { | ||
| private void verifyApplicationEnabled(String appId) { |
There was a problem hiding this comment.
I still think that this method brings and always brought some fragility. It is completely heuristic, while we should rather use some lifecycle stages of the application to check where we are and what we can do .... sounds like sci-fi now :-)
I think there are also other places feeling this pain in GF.
There was a problem hiding this comment.
And second question: appId == null are illegal always or found some legal cases where it may be null? You always thow IllegalStateException.
There was a problem hiding this comment.
Before appId == null was legal when JNDI context was cleared or not propagated. Since I copy the value of appName into the context even in those cases, it should now never be null.
There was a problem hiding this comment.
I still think that this method brings and always brought some fragility. It is completely heuristic, while we should rather use some lifecycle stages of the application to check where we are and what we can do .... sounds like sci-fi now :-)
I think there are also other places feeling this pain in GF.
I have a POC of a better solution here: #24544
There, I use an app lifecycle listener to trigger asynchrouns tasks after an app is deployed. We could extend that approach also to find out whether the application is enabled or being deployed. But even this PR reduces some magic - the appName is always present and the only magic now is that when the Application object doesn’t exist, it’s assumed that deployment is in progress.
There was a problem hiding this comment.
But #24544 requires some more work to finalise the solution. It’s not a priority for me now, I raised it only to share the idea with you and store it for future. I’m going to work on some other fixes/improvements, which are more important now.
There was a problem hiding this comment.
Sorry, I’m on phone, I meant this PR: #24540
...concurrent-impl/src/main/java/org/glassfish/concurrent/runtime/ContextSetupProviderImpl.java
Show resolved
Hide resolved
Signed-off-by:Ondro Mihalyi <[email protected]>
Signed-off-by:Ondro Mihalyi <[email protected]>
Reverted now. |
OndroMih
changed the title
This addresses the conversation in #24539 (comment) and fixes #21087.
Changes the logic in ContextSetupProviderImpl to always store the invocation context for another thread, and then restore it only if needed. The application name is retrieved from the stored context, so it's always available and should never be
null.I also fixed the problem that appName was sometimes null by always copying the appName from the previous context to the new one.
Passes Concurrency TCK in appserver/tests/tck/concurrency. Passes tests in appserver/tests/admin and application. Although 2 tests failed when I ran locally, I rerun the tests individually and they passed. One in Concurrency TCK failed because GlassFish failed to start, one in Admin failed because it parsed the log file incorrectly (org.glassfish.main.admin.test.progress.DetachAttachITest) and expected a number where it was text.- Probably because of my slower machine there was some difference in the logs and possibly some extra log lines. I saw something similar in
ProgressStatusComplexITest.java, which I fixed in this PR, but inDetachAttachITest.javait wasn't clear how to fix and running the test separately passed the test.I also created a draft of a more complex improvement to execute the tasks after deployment is complete: #24540. There are multiple options