Tests to verify that tasks submitted to executor during deployment are executed #24539
Conversation
OndroMih
changed the title
Triggers the tasks immediately, even before the deployment completes. To do: * consider whether it's better to execute the tasks after deployment completed Signed-off-by:Ondro Mihalyi <[email protected]>
OndroMih
force-pushed
the
ondromih-executors-during-deployment
branch
from
6eaeb21
to
cf0c624
Compare
|
The failed test ProgressStatusComplexITest doesn't seem to be related to my changes. Can you please check, @dmatej or restart the build? |
3 failures are weird, does it pass locally? |
| @@ -250,14 +250,14 @@ private ComponentInvocation getSavedInvocation() { | |||
| */ | |||
| private boolean isApplicationEnabled(String appId) { | |||
| if (appId == null) { | |||
| // we don't know the application name yet, it is starting. | |||
| return true; | |||
| return false; | |||
There was a problem hiding this comment.
This change is wrong - the DOL does still some magic. I think I did something similar when I reintroduced concurrency before JEE10 and I broke some other things by this change. I see also my FIXME in the history:
// we don't know the application name yet, it is starting.
// FIXME: it would be good to know the application name even in this phase.
You moved the comment down, now it even doesn't make sense, because you return true instead original false.
And javadoc doesn't make sense too - no exception would be thrown, it would return false. Now it returns true.
There was a problem hiding this comment.
If it all would be written correctly, for null appId it should throw NPE. We have to get there some day :-)
There was a problem hiding this comment.
Returning true at the end makes sende - it we have appName but the Application object isn’t created yet, then the app is being deployed and thus it’s enabled. This actually fixes #21087 and it’s a similar fix to how Payara fixed the same issue a while ago.
There was a problem hiding this comment.
I don’t understand why DOL would call this method with no app name. Then which app is enabled if we return true?
There was a problem hiding this comment.
This change is wrong - the DOL does still some magic. I think I did something similar when I reintroduced concurrency before JEE10 and I broke some other things by this change.
Now I see that a test in application-tests failed exactly because appName is null in ContextSetupProviderimpl (in concurrent-impl). Because invocation.getAppName() is null. I'll try to find out why that happens.
There was a problem hiding this comment.
Here's the reason for invocation.getAppName() == null:
https://github.com/eclipse-ee4j/glassfish/blob/master/appserver/concurrent/concurrent-impl/src/main/java/org/glassfish/concurrent/runtime/ContextSetupProviderImpl.java#L243
By default, no information from the current invocation is saved for a new thread if JNDI isn't propagated. getSavedInvocation() method either returns an empty invocation (if JNDI should be cleared), or null (by defualt). Both are later interpreted as appName == null.
I believe that in both cases, a new invocation should be created and some non-JNDI data should be copied, e.g. app name.
There was a problem hiding this comment.
Or, if we consider the app name part of the JNDI context, then we should at least change the behavior in ContextSetupProviderimpl.setup(), so that it doesn't try to call isApplicationEnabled if appName is null and automatically considers it as enabled.
I don't know what is better, hm.
There was a problem hiding this comment.
OK, I resign for now. I reverted this change. I have an idea how to improve the current concurrency runtime and propagate app context if there's no current context but that would be for another PR.
There was a problem hiding this comment.
Here's my idea how to propagate app context if there's none set up: #24544
I ran just |
|
I ran the whole test suite at night, including the tck, with I sporadically see this error also when I start GlassFish manually, it happens to me maybe 1 out of 100 times. When I restart GlassFish it's fixed. Probably some race condition during startup. I created an issue for it here: #24543 |
appName is null by default in concurrent tasks because app context isn't propagated. This needs to be fixed before appName can be considered not null. Signed-off-by:Ondro Mihalyi <[email protected]>
OndroMih
changed the title
This should not happen, you should receive a job id, not the result. Wasn't it caused by your local changes? |
Maybe. Everything passes on Jenkins and I think I also ran all the admin tests later and everything passed. So I wouldn't bother with it. |
Adds tests for #24544.
This PR was previously raised to fix #21087 but then this was fixed by #24544. What remains in this PR is tests to verify the expected behavior.