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: Close Jetty h2 streams with RST_STREAM and no error code #6441

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Nov 26, 2024

This is a Jetty-specific workaround to avoid irritating the Python gRPC client into failing calls that had already half-closed successfully.

Since we're now using ServletOutputStream.close() in place of AsyncContext.complete(), we need to apply the same wrapping for trailers to close() - that is, when the stream is clsoed, we can't rely on the servlet container sending our trailers because grpc-web trailers are actually a DATA frame which must be explicitly written.

See #6400
Fixes #5996
Reapplies #6401
Backport #6424
Backport #6478

devinrsmith
devinrsmith previously approved these changes Nov 26, 2024
@niloc132 niloc132 marked this pull request as draft December 5, 2024 18:21
@niloc132
Copy link
Member Author

niloc132 commented Dec 5, 2024

Converting to draft, the original fix didnt actually reapply the original fix, git seemed to have decided it wasnt needed...

Reapplying the real fix seems to be adding regular stack traces to the server log when the docker healthcheck runs, will file a bug shortly to track.

…eephaven#6478)

Changes from deephaven#6420 were supposed to be reverted in deephaven#6424, but apparently
git interpreted the branch's relationship with main differently than I
did.

This patch reapplies deephaven#6401, with an additional change to avoid noisy
stack traces from grpc clients that set `grpc-timeout`. At this time,
Deephaven doesn't handle timeouts nor do any default clients set
timeouts, so the impact here should be minimal.

Fixes deephaven#5996 
See deephaven#6400
@niloc132 niloc132 marked this pull request as ready for review December 12, 2024 03:19
@devinrsmith devinrsmith merged commit a002463 into deephaven:rc/v0.37.x Dec 12, 2024
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants