-
Notifications
You must be signed in to change notification settings - Fork 148
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
[Breaking] Allow aborting exec, remove execTask #590
Merged
Merged
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
Instead of synchronously stat'ing the path on ENOENT when it's possible that the caller doesn't care we'll leave it up to callers to choose whether to try to detect why the exec failed. Note that the pathExist approach was subject to race conditions and that there's no guarantee you'd ever be able to figure out exactly why the call failed
sergiou87
approved these changes
Oct 21, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!! This is so much more idiomatic 🥹 👏
Merged
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.
As of Node 20 execFile supports an
AbortSignal
as a means to cancel (i.e. kill) the process. Once the AbortSignal fires Node will take care of sending the specifiedkillSignal
(defaults to SIGTERM) to the process unless the process has already run to completion in which case the abort is a noop.This lets us do away with our custom
execTask
implementation in favor of relying on Node to do the heavy lifting. In order for callers to be able to detect an abort all errors thrown from Dugite'sexec
will now be of typeExecError
and callers will be able to use thecode
property to determine the cause of the error.Example:
This also removes the custom error codes
RepositoryDoesNotExistErrorCode
andGitNotFoundErrorCode
. These were an attempt to provide friendly error codes for implementors but assumed that the callers cared about the distinction between the Git executable missing and the cwd missing. Instead of synchronously stat'ing the path on ENOENT when it's possible that the caller doesn't care we'll leave it up to callers to choose whether to try to detect why the exec failed.Note that the pathExist approach was subject to race conditions and that there's no guarantee you'd ever be able to figure out exactly why the call failed. Here's an example of mimicking RepositoryDoesNotExistErrorCode and GitNotFoundErrorCode:
This also removes the custom message when hitting
maxBuffer
limits. This message is already decent enough straight from Node.