search

googleapis / gax-nodejs

Google API Extensions for Node.js
Apache License 2.0
113 stars 83 forks source link

fix: Only call getResumptionRequestFn once #1593

Closed danieljbruce closed 2 months ago

danieljbruce commented 2 months ago

Source code changes

The current code calls getResumptionRequestFn twice when the result from the first call can be reused. For clients implementing retries, we only want one call to getResumptionRequestFn to be done per request because we don’t want the state that getResumptionRequestFn relies on to be modified twice.

This change intends to reuse the result from the first call to getResumptionRequestFn so that getResumptionRequestFn is only called once.

Test changes

The resumption function is wrapped in sinon.spy so that its call count can be measured in an existing test. The call count is later tested to ensure its value is 1.

conventional-commit-lint-gcf[bot] commented 2 months ago

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot https://conventionalcommits.org/

danieljbruce commented 2 months ago

Upon further inspection of this, it looks like the state updates need to be done earlier in the shouldRetryFn so this is not as important as I originally thought because now getResumptionRequestFn will not update state. Still though, other libraries might be updating the resumption state in getResumptionRequestFn so maybe this still has value.

leahecole commented 2 months ago

If tests are passing, this seems good to me, but I think we should get #1592 merged first just to make sure there's no unexpected side effects of it

leahecole commented 2 months ago

Teat failures likely not related to this PR. Will try rerunning in a bit

Test failures are part of this issue

leahecole commented 2 months ago

Upon further inspection of this, it looks like the state updates need to be done earlier in the shouldRetryFn so this is not as important as I originally thought because now getResumptionRequestFn will not update state. Still though, other libraries might be updating the resumption state in getResumptionRequestFn so maybe this still has value.

I think this does have value and when I was going through the code, I honestly think that the original way this was written was likely a mistake because we don't utilize the resumptionRetryArgument other than to check if it's defined - that feels wrong - so I think this is totally reasonable