Skip to content
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

Fix missing notifyRemoteAsyncErrors http config wiring #11897

Merged
merged 25 commits into from
Jun 18, 2024

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jun 10, 2024

HttpConfiguration.isNotifyRemoteAsyncErrors() is not used anywhere in Jetty 12.

Fix this by adding the necessary check.

@lorban lorban added the Bug For general bugs on Jetty side label Jun 10, 2024
@lorban lorban requested review from joakime and sbordet June 10, 2024 11:38
@lorban lorban self-assigned this Jun 10, 2024
@lorban lorban added the Sponsored This issue affects a user with a commercial support agreement label Jun 10, 2024
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case.

Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet June 11, 2024 14:13
Signed-off-by: Ludovic Orban <[email protected]>
@@ -437,7 +437,7 @@ public Runnable onFailure(Throwable x)
// Notify the failure listeners only once.
Consumer<Throwable> onFailure = _onFailure;
_onFailure = null;
Runnable invokeOnFailureListeners = onFailure == null ? null : () ->
Runnable invokeOnFailureListeners = onFailure == null || !getHttpConfiguration().isNotifyRemoteAsyncErrors() ? null : () ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right place to check for isNotifyRemoteAsyncErrors(), because not all errors that arrive here are remote async errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there is a way to reliably discriminate errors. Maybe this feature is specific to ee* environments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Jetty 11, the check for notifyRemoteAsyncErrors was done in the HTTP/2 and HTTP/3 layers, not generically.

I think we should restore that.

The idea would be to introduce an onRemoteFailure() method that is only called from H2 and H3, and have this new method delegate to onFailure() with an extra parameter, so that pending reads and writes are notified, but listeners are only invoked if the extra parameter is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed an attempt at that; two observations:

  • having that notification filtering logic in core means it's not needed in ee* anymore
  • H3 does not have client resets?!

Comment on lines 202 to 210
protected Session newHttp2ClientSession(Session.Listener listener) throws Exception
{
String host = "localhost";
int port = ((NetworkConnector)connector).getLocalPort();
InetSocketAddress address = new InetSocketAddress(host, port);
FuturePromise<Session> promise = new FuturePromise<>();
http2Client.connect(address, listener, promise);
return promise.get(5, TimeUnit.SECONDS);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the point of the tests in this module is to run for all transports.

This is HTTP/2 specific, so it should be moved to the jetty-http2-tests module.

Copy link
Contributor Author

@lorban lorban Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this isn't an idea place, but the jetty-http2-tests module is under jetty-core, it does not and cannot depend on ee10.

@@ -196,6 +206,29 @@ protected void startClient(Transport transport) throws Exception
client.start();
}

protected Session newHttp2ClientSession(Session.Listener listener) throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as ee10.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same response.

@lorban lorban requested a review from sbordet June 12, 2024 15:50
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

@lorban lorban requested a review from sbordet June 17, 2024 09:24
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all booleans.
Just do the instanceof EOFException in HttpStreamOverHTTP3.onFailure().

{
if (LOG.isDebugEnabled())
LOG.debug("failure on {}", this, failure);
LOG.debug("{}failure on {}", remote ? "remote " : "", this, failure);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LOG.debug("{}failure on {}", remote ? "remote " : "", this, failure);
LOG.debug("{} failure on {}", remote ? "remote" : "local", this, failure);

{
if (LOG.isDebugEnabled())
LOG.debug("Processing stream failure on {}", stream, failure);
LOG.debug("Processing {}stream failure on {}", remote ? "remote " : "", stream, failure);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LOG.debug("Processing {}stream failure on {}", remote ? "remote " : "", stream, failure);
LOG.debug("Processing {} stream failure on {}", remote ? "remote" : "local", stream, failure);

{
session.onSessionFailure(error, reason, failure);
session.onSessionFailure(error, remote, reason, failure);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you keep the parameter order to be [remote, error, reason] like elsewhere?

Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet June 18, 2024 15:27
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban merged commit 9546b3a into jetty-12.0.x Jun 18, 2024
9 checks passed
@lorban lorban deleted the fix/jetty-12.0.x/wire-isNotifyRemoteAsync branch June 18, 2024 18:00
joakime added a commit that referenced this pull request Jun 19, 2024
 (#11897)

Fixed missing notifyRemoteAsyncErrors http config wiring and add tests

Signed-off-by: Ludovic Orban <[email protected]>
@olamy olamy mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants