Skip to content

Catch errors thrown in ensure block#80

Merged
jnicklas merged 1 commit intomasterfrom
catch-ensure-errors
Mar 5, 2020
Merged

Catch errors thrown in ensure block#80
jnicklas merged 1 commit intomasterfrom
catch-ensure-errors

Conversation

@jnicklas
Copy link
Copy Markdown
Collaborator

@jnicklas jnicklas commented Mar 5, 2020

Closes #78

If an error ever occurrs in an ensure block, then that is a serious problem. We should inform the user that this is not good by printing a huge nasty warning to the console.

However, if such a situation does occur, at least it should not leave the process tree in an inconsistent state. Other exit hooks should still run, and other processes should still shut down. Currently if such an error does occur, it will blow the stack, and cause finalization to stop
abrubtly. This is very much not good.

Here we're rescuing any such errors, printing them to the console, but crucially we are NOT re-raising them. This allows us to proceed with finalization, which will leave the process tree in a more consistent state.

It's worth noting though that such a situation really is very undesirable. The Rust language does a hard abort of the process with an error exit code if a destructor panics, we cannot do something similar in effection, since we want to run in browsers, and browsers cannot hard-exit.

@jnicklas jnicklas requested a review from cowboyd March 5, 2020 10:39
@frontsidejack

This comment has been minimized.

@jnicklas jnicklas force-pushed the catch-ensure-errors branch from 6ca7c79 to 9dc5333 Compare March 5, 2020 10:42
@frontsidejack

This comment has been minimized.

@jnicklas jnicklas force-pushed the catch-ensure-errors branch from 9dc5333 to 5867178 Compare March 5, 2020 10:43
@frontsidejack

This comment has been minimized.

@jnicklas jnicklas force-pushed the catch-ensure-errors branch from 5867178 to 8ec127f Compare March 5, 2020 10:45
@frontsidejack
Copy link
Copy Markdown
Member

A preview package of this pull request has been released to NPM with the tag catch-ensure-errors.
You can try it out by running the following command:

$ npm install effection@catch-ensure-errors

or by updating your package.json to:

{
  "effection": "catch-ensure-errors"
}

Once the branch associated with this tag is deleted (usually once the PR is merged or closed), it will no longer be available. However, it currently references effection@0.5.1-312a6d5 which will be available to install forever.

Generated by 🚫 dangerJS against 8ec127f

@jnicklas
Copy link
Copy Markdown
Collaborator Author

jnicklas commented Mar 5, 2020

@cowboyd not sure why the type tests are failing. It looks unrelated to this PR though.

Copy link
Copy Markdown
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

A very much needed change, and looks good to me. I added a suggestion to tweak the message to try and let folks know the consequences of a failed exit hook, and to reference the ensure() API since we don't use exit handlers

Comment thread src/context.js
@cowboyd
Copy link
Copy Markdown
Member

cowboyd commented Mar 5, 2020

@cowboyd not sure why the type tests are failing. It looks unrelated to this PR though.

I'm guessing that it's because we have our TS config pointed to esnext which is a moving target..... which moved. Maybe if we peg it to es2019 or some fixed target?

@frontsidejack
Copy link
Copy Markdown
Member

A preview package of this pull request has been released to NPM with the tag catch-ensure-errors.
You can try it out by running the following command:

$ npm install effection@catch-ensure-errors

or by updating your package.json to:

{
  "effection": "catch-ensure-errors"
}

Once the branch associated with this tag is deleted (usually once the PR is merged or closed), it will no longer be available. However, it currently references effection@0.5.1-2c504ef which will be available to install forever.

Generated by 🚫 dangerJS against 1ec6b7a

Closes #78

If an error ever occurrs in an ensure block, then that is a serious
problem. We should inform the user that this is not good by printing a
huge nasty warning to the console.

However, if such a situation does occur, at least it should not leave
the process tree in an inconsistent state. Other exit hooks should still
run, and other processes should still shut down. Currently if such an
error does occur, it will blow the stack, and cause finalization to stop
abrubtly. This is very much not good.

Here we're rescuing any such errors, printing them to the console, but
crucially we are NOT re-raising them. This allows us to proceed with
finalization, which will leave the process tree in a more consistent
state.

It's worth noting though that such a situation really is very
undesirable. The Rust language does a hard abort of the process with an
error exit code if a destructor panics, we cannot do something similar
in effection, since we want to run in browsers, and browsers cannot
hard-exit.
@jnicklas jnicklas force-pushed the catch-ensure-errors branch from 1ec6b7a to 1ab9ce5 Compare March 5, 2020 14:53
@frontsidejack
Copy link
Copy Markdown
Member

A preview package of this pull request has been released to NPM with the tag catch-ensure-errors.
You can try it out by running the following command:

$ npm install effection@catch-ensure-errors

or by updating your package.json to:

{
  "effection": "catch-ensure-errors"
}

Once the branch associated with this tag is deleted (usually once the PR is merged or closed), it will no longer be available. However, it currently references effection@0.5.1-5826040 which will be available to install forever.

Generated by 🚫 dangerJS against 1ab9ce5

@jnicklas jnicklas merged commit d03a5ca into master Mar 5, 2020
@jnicklas
Copy link
Copy Markdown
Collaborator Author

jnicklas commented Mar 5, 2020

:shipit:

@jnicklas jnicklas deleted the catch-ensure-errors branch March 5, 2020 14:56
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.

Throwing an error in an ensure() hook can leave effection tree in an unstable state

3 participants