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: do not release client before retry #870
Conversation
Codecov Report
@@ Coverage Diff @@
## master #870 +/- ##
=========================================
- Coverage 94.9% 94.9% -0.01%
=========================================
Files 25 25
Lines 15198 15200 +2
Branches 1125 1128 +3
=========================================
+ Hits 14424 14425 +1
Misses 767 767
- Partials 7 8 +1
Continue to review full report at Codecov.
|
dev/src/index.ts
Outdated
@@ -1181,6 +1183,7 @@ export class Firestore { | |||
*/ | |||
private _initializeStream( | |||
backendStream: Duplex, | |||
liftime: Deferred<void>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, liftime
vs lifetime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Addresses #829
I think there is a potential problem with stream retry in the existing client. I wasn't able to write a test that hit the scenario, but I can try to put it into words.
In the existing code, requestStream() takes a client object out of the client pool and then calls retry(). There are two Promises that manage all the logic: Result and Lifetime. Since there are separate, it is possible that result.reject() is called (causing a retry) just before lifetime.resolve() is called, which returns the client back to the pool. We then retry with a client that was already returned to the pool, which - if the use is unlucky - is a client that was already garbage collected.
The change in this PR reverses the order: Each retry attempt fetches its own client from the pool, and thus the lifetime is now bound to the length of a single retry attempt.