Add retries for idempotent errors on Session.run() - #1404
Conversation
16109e5 to
68dfa92
Compare
2904c5c to
34c25a9
Compare
| }).catch(onErrorOriginal) | ||
| const onErrorWrapper = (error: Error, runError?: boolean): void => { | ||
| if ( | ||
| runError === true && !this._hasRetried && error instanceof Neo4jError && |
There was a problem hiding this comment.
might be misreading the flow here - _hasRetried is checked inside each wrapper, and _handleError walks the observer list calling them in turn. if someone attaches two consumers to the same result (result.keys() then await result or similar), does that mean the first wrapper sets _hasRetried = true and queues the retry, while the second wrapper sees the flag, falls into the else, releases the connection holder and rejects with the idempotent error?
do you think this is guarded elsewhere, or maybe just "bad usage" and ignore?
There was a problem hiding this comment.
No this is indeed an issue, thank you! I think in my mental model, the fact that we unsubscribe from the stream observer would protect against this, but since the .forEach loop has started it will run the error callback from every observer that was subscribed when it started, even if the first one removes the subsequent entries!
I altered the code to instead of tracking if a result has retried, it tracks the exact error that it retried due to. As all the observers on the result will receive the exact same error object (not just an identical object but the same one) this can be used to safeguard against treating the same error as a 2nd error
StephenCathcart
left a comment
There was a problem hiding this comment.
Other than open question looks good to me 👍
Closes: DRIVERS-309