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 client with stream methodshould return stream object #1101

Merged
merged 13 commits into from
Feb 3, 2022

Conversation

summer-ji-eng
Copy link
Collaborator

@summer-ji-eng summer-ji-eng commented Feb 2, 2022

Problem: client library always return a rejected promise if the client has been closed. However, when stream make an apicall, it expect a stream object not promise object. This cause TypeError of .on is not function.

Fix: return a stream with error event if ongoing stream make on closed client.

@snippet-bot
Copy link

snippet-bot bot commented Feb 2, 2022

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@summer-ji-eng summer-ji-eng marked this pull request as ready for review February 3, 2022 00:41
@summer-ji-eng summer-ji-eng requested a review from a team as a code owner February 3, 2022 00:41
@summer-ji-eng
Copy link
Collaborator Author

Investigate if we need a unit test on this case.

@summer-ji-eng
Copy link
Collaborator Author

summer-ji-eng commented Feb 3, 2022

Bidi stream and client stream return different error from grpc like:

{"code":14,"details":"No connection established","metadata":{}}

Added server stream unit test for closed client. Will investigate bidi and client later.

@alexander-fenster please help me take a look. 🙇

stream.emit('error', new GoogleError('The client has already been closed.'));
});
return stream;
}

Choose a reason for hiding this comment

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

doesn't this need to also happen in the rejection arm? ie if the bigQueryStorageStub is rejected (ie. no creds), the the promise will still get bubbled up to streaming.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the bigQueryStorageStub is rejected, it throw an error(see line 225 below). This wouldn't get bubble up to streaming.js.

In other words, only whenbigQueryStorageStub resolved, and client was terminated, the streaming call would receive Passthrough stream with error event.

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

Please don't release it to all APIs, let's just generate bigtable with this change and see if it works for them, then we can release it across the board.

@summer-ji-eng
Copy link
Collaborator Author

Please don't release it to all APIs, let's just generate bigtable with this change and see if it works for them, then we can release it across the board.

Make sense. I will release the version, but not going to update the google 3 WORKSPACE yet.

@summer-ji-eng summer-ji-eng added the automerge Merge the pull request once unit tests and other checks pass. label Feb 3, 2022
@summer-ji-eng summer-ji-eng merged commit 40f23d1 into main Feb 3, 2022
@summer-ji-eng summer-ji-eng deleted the close_client branch February 3, 2022 19:21
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 3, 2022
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 3, 2022
🤖 I have created a release *beep* *boop*
---


## [2.13.0](v2.12.2...v2.13.0) (2022-02-03)


### Features

* add snippet metadata file ([#1084](#1084)) ([5965e9c](5965e9c))


### Bug Fixes

* close client with stream methodshould return stream object ([#1101](#1101)) ([40f23d1](40f23d1))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.

3 participants