Removed duplicit completion#6098
Conversation
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- Is be visible in test logs Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
…cessing Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
e64f6ec to
0e9c854
Compare
- 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>
0e9c854 to
1a77c3f
Compare
mkarg
left a comment
mkarg
left a comment
There was a problem hiding this comment.
The actual change looks good at me; didn't have time to check the modified tests.
It would be beneficial to have a brief description what actual problem this change fixes.
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
| @Override | ||
| public void complete() { | ||
| if (isCompleted()) { | ||
| LOGGER.log(Level.WARNING, "The processing was already completed!", |
There was a problem hiding this comment.
Do we really want to have a WARNING here (which possibly is forwarded to the end user running Jersey)? IMHO if this message is logged, it proofs a bug in Jersey itself, so it might be better to have it on debug level?
There was a problem hiding this comment.
Everything has consequences. If it would be already in the completed state, server would send the message to the client once more, the second time, when the client doesn't expect it. That was happening, causing weird errors.
However it was not well reproducible and it seems to me there's yet something - but maybe not, because on any hardware I used for testing this PR, I wasn't able to break relevant tests.
I really hate swallowing errors, because then we have just reports in issues, but no clue what really happened, no warning, and we burn lot of time by investigation and reproducing the problem. Same applies to users, it is hard to report something when incorrect situations are silently dropped.
So whatever breaks after this, we know at least something. And if this change is correct, there will be no warnings.
The now deleted listener (and the unit test for it) was added in 431b933 with this commit message:
Prevent NPE in the servlet container by writing the entity when the servlet container is shutdown
However now there are no NPE any more, it was probably fixed too somehow later.
I might be wrong too, but then it is easier to fix it from this state of code base than from the previous one.
There was a problem hiding this comment.
I do not support this policy. Either we assume our code is correct, then the right way to deal with the situation is throwing an AssertionException. Or we assume our code is incorrent, the we should fix it. In case we assume the situation may happen, throwing a RuntimeException (or even a checked exception in case of specified behavior) would be correct. But in no case I support logging a warning given to end users, as it is not his fault. BTW, reducing to debug does not swallow anything. The administrator can enable logging these events, if he likes to.
What happens with such "useless" log entries is shown in the discussion between Lenny and Ondro: It produces complaints from customers, it produces arguments between committers, it does neither prevent a problematic situation nor solve one.
Having said that, kindly asking to not merge PRs as long a discussions are ongoing, in particular in production branches. This is considered unfriendly, as you are ignoring other committer's concerns without any need. Thanks.
There was a problem hiding this comment.
First you approved this PR, this your comment came too late.
While you complain about hypothetical warning which doesn't break features (while exception could and no log at all would cause that we all forget about this for another 5 years until first user complains again), we have much more serious issues and nobody cares about them - 4.x fails randomly in tests, deployment needs to be rewritten, there are branches with no usage, ...
Btw the warning can be easily suppressed as a temporary workaround (if it would happen). With JUL:
org.glassfish.jersey.servlet.async.AsyncContextDelegateProviderImpl.level=OFF
Throwing exceptions have consequences - something would react somehow, throwing exceptions in a completion phase is quite risky.
3 participants
Inspired and based on #6086 I am creating this PR. The main difference is that I removed the listener completely, and replaced the original unit test which used proxies with another using Mockito.
With @guerrillalg we still coordinate in a hunt of HTTP codes 500 and 200 - I cannot reproduce them neither with the original code nor with his nor mine PR.
Quick test
Tested with JDK17, 11 skips as Jetty 12 requires 17+.
Note
The author of the
JettyHttpContainerDuringShutdownTestis @guerrillalg , I just contributed a bit. For the purpose of comparison of both PRs I checked out and committed the test here too.