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: attach a catch handler to the handleContentP promise #250

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

kalinkrustev
Copy link
Contributor

This attaches a catch handler to the handleContentP promise, as in some cases the flush is not called before node decides that this is an unhandled promise rejection.

Fixes #249

This attaches a catch handler to the handleContentP promise, as in some cases the flush is not called before node decides that this is an unhandled promise rejection.
@kalinkrustev kalinkrustev requested a review from a team as a code owner November 22, 2023 13:06
@wraithgar
Copy link
Member

Ah good catch. It looks like the thing that is throwing is

const tmp = await makeTmp(cache, opts)

If that were to be brought inside the try/catch block, and then this line

if (!tmp.moved) {

Changed to

if (!tmp?.moved) {

We wouldn't need the catch.

@wraithgar
Copy link
Member

There is an error handler set up on the promise at

(er) => cb(er)

But it only happens during flush, way too late.

@@ -67,6 +67,7 @@ class CacacheWriteStream extends Flush {
this.cache,
this.opts
)
this.handleContentP.catch(error => this.destroy(error))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.destroy seems to be overreaching in scope. I think we want cb(error)

We can also attach it directly to the function call handleContent().catch()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this does not help.

Copy link
Contributor Author

@kalinkrustev kalinkrustev Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.destroy seems to be overreaching in scope

Yes, I changed it to emit the error instead. Seems to work and does not break the test coverage.

@kalinkrustev
Copy link
Contributor Author

kalinkrustev commented Nov 23, 2023

We wouldn't need the catch.

Why? It will still be a rejected promise. It is currently a try...finally block. If it is changed to try...catch, the error should still go somewhere. And it could be also error coming from pipeToTmp or moveToDestination.

@wraithgar
Copy link
Member

Looks good w/ just the simple .catch added. Were you able to test this against your example in #249?

@kalinkrustev
Copy link
Contributor Author

Yes, I tested it and it works fine.

@wraithgar wraithgar merged commit 6755c0e into npm:main Nov 27, 2023
15 checks passed
@github-actions github-actions bot mentioned this pull request Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] put.stream can crash the process with unhandled exception, even when error handler is attached
2 participants