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: don't emit error event during stream handoff #1592

Merged
merged 3 commits into from
May 8, 2024

Conversation

leahecole
Copy link
Contributor

fixes b/338064353

OLD BEHAVIOR: in streamingRetryRequest.ts as part of onResponse, an 'error' event would be emitted while the retryStream is being destroyed. This event (usually the first error event in a retry sequence) would be bubbled up to the caller as part of the stream handoff that happens in retries where the old stream is destroyed, a new one is created, and events are forwarded. The call would still resume afterwards and continue successfully, but that error would be raised when it shouldn't have been

NEW BEHAVIOR: The stream is still destroyed, but no error event is emitted. Stream handoff is the same as before, call continues, and the only errors that would be bubbled up to the caller should be ones that are not retry eligible.

This PR makes that one line behavior change and adjusts the test-application to expect that behavior. Our tests beforehand were doing error handling that is atypical of what a client would do (we were either ignoring errors or checking info about them before raising them, which should be handled at the gax level)

Major thanks to @danieljbruce for noticing the resulting behavior and raising it.

@leahecole leahecole requested review from a team as code owners May 2, 2024 17:22
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 2, 2024
Copy link
Contributor

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

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

Thanks @leahecole. I don't know much about the internals of gax, but the outcome in the description is exactly what we need for retries to work upstream.

@leahecole
Copy link
Contributor Author

I'll take a look at the system test failure on Monday!

@leahecole leahecole marked this pull request as draft May 3, 2024 20:05
@leahecole leahecole marked this pull request as ready for review May 8, 2024 03:00
@leahecole leahecole requested review from sofisl and removed request for alexander-fenster May 8, 2024 17:50
// retryStream must be destroyed here for the stream handoff part of retries to function properly
// but the error event should not be passed - if it emits as part of .destroy()
// it will bubble up early to the caller
retryStream.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Future idea, this will be a great place to add some tracing whenever we add that.

Copy link
Contributor

@sofisl sofisl left a comment

Choose a reason for hiding this comment

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

Thanks for the super clear explanation in the PR description! Code is really easy to understand!

@leahecole leahecole added the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
@leahecole leahecole added the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
@leahecole leahecole merged commit 2e7d30a into googleapis:main May 8, 2024
18 checks passed
@release-please release-please bot mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants