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: return error if GetOperation call fails #1304

Merged
merged 2 commits into from
Aug 17, 2022
Merged

Conversation

alexander-fenster
Copy link
Contributor

Fixes googleapis/nodejs-video-intelligence#523.

When a long running operation starts, we start polling for result by sending GetOperation calls. Apparently, if a GetOperation call itself fails (e.g. with 8 RESOURCE_EXHAUSTED), this error is never handled properly, resulting in an unhandled rejection.

The problem is in the following piece of code:

const noCallbackPromise = this.currentCallPromise_!.then(responses => {
self.latestResponse = responses[0] as LROOperation;
self._unpackResponse(responses[0] as LROOperation, callback);
return promisifyResponse()!;
});

In the created promise, the promisifyResponse call handles the case where GetOperation returned success but the operation itself has failed, but the case where GetOperation itself fails is not handled. I'm adding error handling in this place by supplying the second parameter to .then() and calling the provided callback if it's set, and rejecting the promise if not. In the real life use case, the callback is supplied:

self.getOperation((err, result, metadata, rawResponse) => {
if (err) {
setImmediate(emit, 'error', err);
return;
}

I added a unit test for this fix, and tested it locally with the real quota errors returned by Video Intelligence API.

@alexander-fenster alexander-fenster requested a review from sofisl July 26, 2022 19:55
@alexander-fenster alexander-fenster requested a review from a team as a code owner July 26, 2022 19:55
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jul 26, 2022
@alexander-fenster
Copy link
Contributor Author

Just an update - this fix broke our integration tests, which may or may not mean that something else is broken. I'm looking deeper. The fun thing is that the integration tests worked (calls returned correct results), but some errors were not caught, and with this fix they are being caught now. I need to figure out if it's a bug in the tests, or in the main code.

@alexander-fenster
Copy link
Contributor Author

This should go in after #1310 fixes the broken tests.

@alexander-fenster alexander-fenster merged commit cb21ced into main Aug 17, 2022
@alexander-fenster alexander-fenster deleted the polling-exception branch August 17, 2022 22:42
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.

Cannot catch RESOURCE_EXHAUSTED Quota exceeded error
2 participants