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

Identify retryable Http2Exception(s) generated by netty #1314

Merged
merged 2 commits into from
Jan 13, 2021

Conversation

idelpivnitskiy
Copy link
Member

Motivation:

HTTP/2 codec may throw an exception when it creates a stream in some cases:

  • When there is a concurrency between closing the parent h2 connection and
    creating a new stream;
  • When stream IDs are exhausted for this endpoint;
  • When maximum active streams violated for this endpoint;
    In all this cases it's safe to retry the request because we do not start
    writing it on the wire. When parent connection is closing concurrently, we
    won't hit this error again because it will be marked as "closing" and LB
    won't select this connection again.

Modification:

  • Wrap Http2Exception(s) with Http2Error.REFUSED_STREAM as retryable;
  • Wrap Http2NoMoreStreamIdsException as retryable;

Result:

HttpClient will automatically retry recoverable HTTP/2 exceptions.

Motivation:

HTTP/2 codec may throw an exception when it creates a stream in some cases:
- When there is a concurrency between closing the parent h2 connection and
creating a new stream;
- When stream IDs are exhausted for this endpoint;
- When maximum active streams violated for this endpoint;
In all this cases it's safe to retry the request because we do not start
writing it on the wire. When parent connection is closing concurrently, we
won't hit this error again because it will be marked as "closing" and LB
won't select this connection again.

Modification:

- Wrap `Http2Exception`(s) with `Http2Error.REFUSED_STREAM` as retryable;
- Wrap `Http2NoMoreStreamIdsException` as retryable;

Result:

`HttpClient` will automatically retry recoverable HTTP/2 exceptions.
// - Stream IDs are exhausted for this endpoint.
// - Maximum active streams violated for this endpoint.
// - Http2ChannelClosedException
return cause.error() == REFUSED_STREAM
Copy link
Member

Choose a reason for hiding this comment

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

There is code in AbstractH2DuplexHandler that also checks for REFUSED_STREAM and throws a Retryable exception. is there a way to have these two areas share code so the relationship is more clear and less likely to miss cases in the future (e.g. if we want to add ENHANCE_YOUR_CALM or others)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it but did not found a good way to do that because in AbstractH2DuplexHandler it handles a user-event and created a new exception based on that event when here we intercept write failures. May give another round of thoughts after the release.

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up with the fresh head :)
#1318

@idelpivnitskiy idelpivnitskiy merged commit db657c3 into apple:main Jan 13, 2021
@idelpivnitskiy idelpivnitskiy deleted the retryable-h2 branch January 13, 2021 07:34
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