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

Add gracefulCancel option #1109

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Add gracefulCancel option #1109

merged 1 commit into from
Jun 3, 2024

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Jun 1, 2024

Fixes #1106.

This adds an abortSignal option. The AbortSignal is shared with the subprocess, which retrieves it using getAbortSignal(). This enables graceful termination, since signals (like SIGTERM) handlers do not work on Windows.

This also adds error.isAborted, which is true when abortSignal was aborted.

@ehmicky ehmicky force-pushed the graceful-cancel branch 3 times, most recently from f1ea3a5 to a0cc654 Compare June 1, 2024 03:15
@sindresorhus
Copy link
Owner

What happened to the gracefulCancelSignal naming? I think having both abortSignal and cancelSignal will be confusing.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 1, 2024

I renamed it to abortSignal, but I can change it back to gracefulCancelSignal if you want?
I found getGracefulCancelSignal() and error.isGracefullyCanceled to be slightly verbose.
I was also considering other names like: haltSignal, stopSignal, disposeSignal.

Which name do you prefer? I'll update the PR then.

@sindresorhus
Copy link
Owner

Have you considered just using cancelSignal and add a gracefulExit option to indicate that it should be graceful?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 1, 2024

Yes, that would work too. 👍 Using both signal termination and graceful termination does not seem to make much sense, so they are rather exclusive from each other. Would you prefer this?

However, the name should probably highlight that the subprocess will still fail (i.e. Execa promise is rejected), even if its exit is being handled. gracefulExit might give the impression the subprocess succeeds. Let's call it maybe gracefulCancel instead? (Since the other option is called cancelSignal.)

@sindresorhus
Copy link
Owner

Using both signal termination and graceful termination does not seem to make much sense, so they are rather exclusive from each other. Would you prefer this?

Yes

Let's call it maybe gracefulCancel instead?

👍

@ehmicky ehmicky changed the title Add abortSignal option Add gracefulCancel option Jun 3, 2024
@ehmicky ehmicky force-pushed the graceful-cancel branch 2 times, most recently from b15a687 to 336efff Compare June 3, 2024 03:49
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 3, 2024

Fixed the PR. 👍

The API is now:

  • gracefulCancel boolean option
  • getCancelSignal() method
  • error.isGracefullyCanceled boolean property

}

throw error;
}
```

## Graceful termination

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe mention that if you write your own scripts just for Unix platforms, a user could also just use SIGTERM, that this is specifically for cross-platform scripts that should also work on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just added the following line:

This is cross-platform. If you do not need to support Windows, signal handlers can also be used.

It redirects to the following section:

On Unix, most signals (not SIGKILL) can be intercepted to perform a graceful exit.

process.on('SIGTERM', () => {
	cleanup();
	process.exit(1);
})

Unfortunately this usually does not work on Windows. The only signal that is somewhat cross-platform is SIGINT: on Windows, its handler is triggered when the user types CTRL-C in the terminal. However subprocess.kill('SIGINT') is only handled on Unix.

Execa provides the gracefulCancel option as a cross-platform alternative to signal handlers.

Retrieves the [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) shared by the [`cancelSignal`](#optionscancelsignal) option.

This requires the [`gracefulCancel`](#optionsgracefulcancel) option to be `true`.

Copy link
Owner

Choose a reason for hiding this comment

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

I know you link to the "more info", but would be nice to just mention here that this method should only be called in a subprocess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the following line:

This can only be called inside a subprocess.

@sindresorhus
Copy link
Owner

I would mention this in the "Features" section in the readme. "Cross-platform graceful termination".

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 3, 2024

Fixed 👍

I have added it in the Features section as part of the following bullet point:

  • Improved Windows support: shebangs, PATHEXT, graceful termination, and more.

@ehmicky ehmicky requested a review from sindresorhus June 3, 2024 18:11
@sindresorhus sindresorhus merged commit d8190e5 into main Jun 3, 2024
14 checks passed
@sindresorhus sindresorhus deleted the graceful-cancel branch June 3, 2024 23:39
@sindresorhus
Copy link
Owner

Great work! I'm very happy with what we ended up with 👍

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.

Graceful termination
2 participants