Create streams only after Promise.all() #608
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: alxndrsn <alxndrsn> (cherry picked from commit a3a07f6)
Related to #482. This prevents the following sequence: - A stream is created within a Promise.all(). - The stream is to be processed after the Promise.all() is fulfilled. - However, another promise in the Promise.all() is rejected. - As a result, the Promise.all() as a whole is rejected. - As a result, the stream is never processed and isn't destroyed.
matthew-white
commented
Sep 15, 2022
| @@ -36,8 +37,7 @@ const zipPart = () => { | |||
| // if the final component in the pipeline emitted the error, archiver would then | |||
| // emit it again, but if it was an intermediate component archiver wouldn't know | |||
| // about it. by manually aborting, we always emit the error and archiver never does. | |||
| const zipStreamFromParts = (...zipParts) => { | |||
| let completed = 0; | |||
| const zipStreamFromParts = (...zipPartFns) => { | |||
There was a problem hiding this comment.
I recommend hiding whitespace when viewing the diff for this file.
| resultStream.finalize(); | ||
| }); | ||
| } | ||
| resolve(zipPartFns.shift()()) |
This was referenced Sep 15, 2022
|
I've marked this PR as a draft while we finish working on #564. |
|
Closing now that we've decided to close #634. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of a possible solution for #482, copied in part from changes in #564. See the commit message of the fourth commit for notes. The first commit is from #604. I've also cherry-picked from #542 and #553 in order to prevent a conflict with the
masterbranch: this PR touches some of the same files as those PRs. When reviewing this PR, I recommend focusing on the fourth commit.What has been done to verify that this works as intended?
In addition to tests, I've deployed this PR to the QA server and followed the reproduction steps in #482. I was successfully able to send 10 concurrent requests. When I sent 11 concurrent requests, one quickly resulted in an error response ("Completely unhandled exception: timeout exceeded when trying to connect"). That is expected given that the maximum number of database connections is 10. However, what's exciting is that after the 11 responses, there was no database connection leak. 🎉
I still want to write a couple more tests to get at the Promise-related behavior of
zipStreamFromParts()that I've introduced.Why is this the best possible solution? Were any other approaches considered?
I'm actually not sure that this is the best solution or that we want to ship this with v1.5.3. I've created this PR in part so that we can consider this solution further. See #604 (comment) for the three options we've been discussing for v1.5.3.
The main alternative to this solution is a utility function like the one at #482 (comment). In most places, I think this solution is clearer to reason about than what we would get with a utility function. I think the main exception is
zipStreamFromParts(), which has become more complicated. I also think a utility function could be more performant (though I doubt by much). That said, some of the changes that #564 makes are similar to those here, so we might be heading in this direction for multiple reasons.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The hope is that this PR fixes connection leak issues. However, there's a risk of regression with each of the three streaming endpoints that the PR modifies. I think running @alxndrsn's benchmarker against this PR would help us gain confidence.
Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.
No changes needed.
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes