6044 Do not complete requests on receiving notification events#6086
6044 Do not complete requests on receiving notification events#6086guerrillalg wants to merge 10 commits into
Conversation
|
Hi @guerrillalg , thanks for the contribution, however to merge your contribution we need you to create an account on the Eclipse website, and sign ECA License Agreement. Another problem is that your change failed to compile on CI. Version 3.1 must work with both Java 11 and 21. I can try locally later, perhaps it is a CI issue. Btw, catching and ignoring Throwable always smells badly. The feature change - your change goes against 431b933 as you described in the issue. Now, listener methods will not call this method and set just the completed value. I am not sure if that is desired - what completes the context?: @Override
public void complete() {
completed.set(true);
final AsyncContext asyncContext = asyncContextRef.getAndSet(null);
if (asyncContext != null) {
asyncContext.complete();
}
}Maybe the listener needs just to check if it did not complete already? Once it reaches completed state, none of the code inside could be executed again. I am just thinking about it, it doesn't mean you are wrong. |
|
Thanks for looking into the PR, David.
Yes, that's exactly the change I propose. The thing that is going on is that Context is already completing. Jetty is shutting down in our specific case. And during this shutdown context tries to notify all listeners about this. And what I claim to be wrong in the release branch is that a listener was calling back the asyncContext.complete(). So, my proposal is that since it is a listener, it should only listen to events coming in and update its internal state and not to try to complete the context. All tests seem to be fine with this change and semantically it sounds correct as well. |
|
Now Java 21 passed, but with 11 we have this - seems you need an older jetty: Looking here, Jetty 12 requires Java 17. EDIT: I have committed the switch to Jetty 11 in the new test. |
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- more strict - more requests - higher load - removed swallowing exceptions - counting and logging results - higher shutdown timeout after the test - reliable short timeout in between processing Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
|
Well, I had to check it out locally:
The test has same results with the current and the old implementation, so I tend to merge the change because it doesn't break anything (and it seems it doesn't fix anything too, it is just nicer). I made the test more strict, so if there would be anything unexpected, the test should fail. I added also logs, so you can see the sequence. |
|
Yet one idea - I added some log to the complete() method; while the original code was logging this warning, your code was without that. Basically it seems to me that the listener could be completely removed. I can try that tomorrow ... |
# Conflicts: # tests/integration/jersey-6044/pom.xml # tests/integration/jersey-6044/src/main/java/org/glassfish/jersey/tests/jettyresponseclose/Resource204.java # tests/integration/jersey-6044/src/test/java/org/glassfish/jersey/tests/jettyresponseclose/JettyHttpContainerDuringShutdownTest.java # tests/integration/pom.xml
|
Hi @dmatej thanks for your comments. I have finally been able to come back to this issue. I can easily reproduce the problem as of now.
I have also tried to downgrading jetty to version 11 so that a test would be J11 compatible, however it seems that it is only a problem of jetty 12. At least, whenever I do so, test is always working fine, so a test so far is configured to only be executed on a j17. |
| assertThrows(TimeoutException.class, () -> shutdownServer(100)); | ||
| for (Future<Response> responseFuture : waitingResponses) { | ||
| try (Response response = responseFuture.get(30, TimeUnit.SECONDS)) { | ||
| assertThat("Unexpected response code", response.getStatus(), Matchers.anyOf(equalTo(204), equalTo(500))); |
There was a problem hiding this comment.
500 should not be acceptable. Can you find some stacktrace what happened?
|
The test is incompatible with Jetty 11, you have to use Jetty 12, but that requires Java 17, so you have to annotate the test with The test failed on CI when executed with JDK 11. EDIT: Skipping the test is not enough, we have to exclude whole module or at least the compilation. |
|
@guerrillalg Can you test and review my PR?
Especially what I would like to know is
|
- The original test was created by guerrillakg, and was squashed and further updated by David Matejcek. - Both users signed ECA. Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
2 participants
This is a proposal to fix an issue #6044 in CompletedAsyncContextListener. Semantically speaking, this is supposed to be a listener. It is being notified about certain events when complete or error is already happening. It is essentially incorrect to complete the ongoing request at this moment. I propose to only update a flag, which is needed in related code.
Normally, a similar fix should be done in the 2.x as well, however I was not able to reproduce it using a test in that version. We can discuss on this and see how should we deal with it then.