-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
onCompleteFailure called multiple times #9476
Comments
Jetty 9.x is now at End of Community Support. See You should be upgrading to Jetty 10 or Jetty 11 at this point. |
Our system is JDK8, can not upgrade to a higher version, can only use jetty9 |
You should definitely read #7958 and how to get Jetty 9.x support. |
I do understand that you can't support multiple major versions in parallel, but you have to end support at some point. Where I'm not following, is, that a NullPointerException had recently been introduced (with v9.4.51, which means it got broken by an actively performed change, i.e. while in EOL) and nonetheless you don't want to take care of it. |
I agree. Also recently upgraded to 9.4.51 in pre-prod (e.g. loadtest) where we have seen the NPE. If this is some sort of regression recently introduced, it should be fixed in another 9.4.x based release. |
Since we must have introduced in recent release, we will look at fixing it as well. |
Any news? |
@AN3Orik since Jetty 9.x is End of Community Support, this is extremely low priority. Our overwhelming focus is on Jetty 12, with support for Jetty 10 and Jetty 11 right now. |
This is reported against Which was around the release of Jetty 9.4.12. |
It's start happening after changes in this commit i think Failed callback added and this callback is null when AbstractConnection.failedCallback() method reached |
@AN3Orik You must manually add it for it to ever be used, so unless you have started using this handler in |
Added a null check for the Callback parameter in o.e.j.iAbstractConnection.failedCallback(Callback, Throwable) so that the subsequent code-flow is not aborted; while the root cause may be ascertained in case debug logging is enabled. Further, in the case of a RejectedExecutionException when submitting the failCallback, the Runnable is run instead of directly invoking the Callback, so that exceptions in the Callback do not abort the synchronous code-flow.
@gregw Please review the PR #9932 I have submitted to avoid a null Thank you very much. |
…lback 1. Fixed the usages of `Callback` returned by `o.e.j.s.HttpConnection.SendCallback.release()` to check for `null` before invoking the `failedCallback(Callback, Throwable)` or `succeeded()` methods. 2. Also, added a try-finally block around the Callback invocations in order to ensure that shutdownOutput will get performed irrespective of whether they end up throwing any exception.
…w feedback Partially reverted `o.e.j.i.AbstractConnection.failedCallback(Callback, Throwable)` to not have a null check for the `Callback` parameter, based on review feedback from @gregw. This avoids changing the contract of the method parameter (from `@NonNull` to `@Nullable`).
@sbordet @lachlan-roberts I think this is actually a real bug in IteratingCallback in all our releases. I think we are able to call |
@sbordet @lachlan-roberts @lorban The following "test" is passing when it should not. It shows that we can call @Test
public void testMultipleFailures() throws Exception
{
AtomicInteger process = new AtomicInteger();
AtomicInteger failure = new AtomicInteger();
IteratingCallback icb = new IteratingCallback()
{
@Override
protected Action process() throws Throwable
{
process.incrementAndGet();
return Action.SCHEDULED;
}
@Override
protected void onCompleteFailure(Throwable cause)
{
super.onCompleteFailure(cause);
failure.incrementAndGet();
}
};
icb.iterate();
assertEquals(1, process.get());
assertEquals(0, failure.get());
icb.failed(new Throwable("test1"));
assertEquals(1, process.get());
assertEquals(1, failure.get());
icb.succeeded();
assertEquals(2, process.get());
assertEquals(1, failure.get());
icb.failed(new Throwable("test1"));
assertEquals(2, process.get());
assertEquals(2, failure.get()); |
Change state to FAILED from PENDING state
Thanks a lot @gregw! |
Change state to FAILED from PENDING state
merged through to 12.0.x @forfreeday We are 99% sure this will fix your problem, but we did not have an exact reproducer of your exact case. Any chance you can test this out before we do a release? |
@gregw are you still looking for someone to try this out pre-release? I can reproduce this NPE on If you need a guinea pig let me know what 9.x snapshot I should try. |
@markkolich that would be good. I think the most recent nightly is https://oss.sonatype.org/service/local/repo_groups/jetty-with-staging/content/org/eclipse/jetty/jetty-distribution/9.4.52-SNAPSHOT/jetty-distribution-9.4.52-20230807.110254-149.tar.gz else you can build it from source. |
Jetty version(s)
Jetty 9.4.51
Java version/vendor
(use: java -version)
java version 1.8.0_161
OS type/version
CentOS
Description
When I upgrade to Jetty 9.4.51, this exception will appear in the log every day.
What caused this anomaly?
The text was updated successfully, but these errors were encountered: