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 close client if we've already aborted #968

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

bouk
Copy link
Collaborator

@bouk bouk commented Nov 23, 2023

grpc-web throw an error if you close the client after it's already been closed:

https://github.com/improbable-eng/grpc-web/blob/1d9bbb09a0990bdaff0e37499570dbc7d6e58ce8/client/grpc-web/src/client.ts#L327

This can happen with the abort signals if the observer is finished first and then the abort signal is triggered, causing an exception. Here I'm changing the code generator to remove the abort signal listener in the observer finish handler.

@stephenh stephenh changed the title Don't close client if we've already aborted fix: Don't close client if we've already aborted Nov 24, 2023
@stephenh
Copy link
Owner

Cool, thanks sense!

@stephenh stephenh merged commit 05bccc7 into stephenh:main Nov 24, 2023
6 of 7 checks passed
@bouk
Copy link
Collaborator Author

bouk commented Nov 27, 2023

@stephenh thx for the quick review! Would appreciate a release

@stephenh
Copy link
Owner

@bouk ah shoot, those are supposed to happen automatically but looks like the commit message was missing the fix: prefix (kinda annoying but github won't make the PR title overwrite the commit message when there is only a single commit).

I just force-pushed your commit with the fix: prefix added, so it should auto-release in the next few minutes.

stephenh pushed a commit that referenced this pull request Nov 28, 2023
## [1.164.2](v1.164.1...v1.164.2) (2023-11-28)

### Bug Fixes

* Don't close client if we've already aborted ([#968](#968)) ([7ee1507](7ee1507))
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