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

Implements client-side handling of RST_STREAM(NO_ERROR) in HTTP/2. #11054

Open
wants to merge 6 commits into
base: jetty-12.0.x
Choose a base branch
from

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Dec 13, 2023

HTTP/3 could have the same handling, but apparently there is no way to receive a RESET_STREAM or STOP_SENDING frame in Quiche.

Now when receiving a RST_STREAM(NO_ERROR), it is not treated as a failure.

HTTP/3 could have the same handling, but apparently there is no way to receive a RESET_STREAM or STOP_SENDING frame in Quiche.

Now when receiving a RST_STREAM(NO_ERROR), it is not treated as a failure.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from lorban and gregw December 13, 2023 13:09
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
…e completed successfully.

Also, disposing the stream if the request failed, as we cannot rely on the RST_STREAM sent by the server.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet marked this pull request as ready for review December 13, 2023 17:48
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

just some initial comments/questions at this stage.

@@ -775,6 +775,12 @@ public void close()
}
}

public boolean dispose()
Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc please.
Does it need to be public?

@@ -494,16 +494,17 @@ public void succeeded()
{
if (LOG.isDebugEnabled())
LOG.debug("HTTP3 Response #{}/{}: unconsumed request content, resetting stream", stream.getId(), Integer.toHexString(stream.getSession().hashCode()));
stream.reset(HTTP3ErrorCode.REQUEST_CANCELLED_ERROR.code(), new IOException("unconsumed content"));
stream.reset(HTTP3ErrorCode.NO_ERROR.code(), new IOException("unconsumed content"));
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use CONTENT_NOT_CONSUMED?

@sbordet sbordet requested a review from gregw December 14, 2023 23:42
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Server side looks OK... but some questions.
Will need to work out why it is hanging in CI

@gregw
Copy link
Contributor

gregw commented Dec 15, 2023

@sbordet how about in this release cycle we only change the server side to send the reset(NO_ERROR) and leave the client side as is.
We can then think a bit more about the client side changes in the next cycle?

@sbordet sbordet self-assigned this Dec 15, 2023
@sbordet
Copy link
Contributor Author

sbordet commented Dec 15, 2023

@sbordet how about in this release cycle we only change the server side to send the reset(NO_ERROR) and leave the client side as is. We can then think a bit more about the client side changes in the next cycle?

Agreed, see #11069.

@sbordet sbordet changed the title Implemented proper handling of RST_STREAM(NO_ERROR) in HTTP/2. Implements client-side handling of RST_STREAM(NO_ERROR) in HTTP/2. Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants