Skip to content

Conversation

@mitja-kleider
Copy link

PR Type

Bug Fix

PR_TYPE

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

If the remote gracefully shuts down a HTTP/2 connection by sending GO_AWAY, the corresponding h2 error is returned.
Before this change, the connection remained in the pool and using it again results in the same h2 error.
With this change the connection is released from the pool as intended by the remote.

@mitja-kleider
Copy link
Author

I found it difficult to write a suitable test, we would need to shut down the h2 connection gracefully after it has been established and before sending the response. Do you have an idea how to achieve that?

@kevin-logan
Copy link
Contributor

I've just found this PR, which is resolving an issue very similar to one I found: #3788

Unfortunately I don't expect your fix here would resolve my issue, but it's the same underlying problem: that all non-IO errors are ignored when releasing a connection, but some non-IO errors are permanent.

My proposed fix is similar to yours but doesn't bother to check if it's a remote GO_AWAY (as local GO_AWAY errors are precisely my issue) and to ensure the same is done on error handling for both io.poll_ready and io.send_request.

Similarly I found it very difficult to write a test to show the issue. I've managed to write one that is flaky (to demonstrate with some confidence there is indeed an issue) but nothing deterministic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants