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

Prevent application crash when spawn fails #8614

Merged
merged 23 commits into from Nov 14, 2019
Merged

Conversation

niik
Copy link
Member

@niik niik commented Nov 11, 2019

Description

This PR bumps dugite to v1.87.3, a patch release containing desktop/dugite#381. The dugite bump is necessary in order for Desktop to be able to properly handle errors related to process spawning.

If the operating system prevented us from launching a processes due to resource exhaustion (EAGAIN), permissions problems (EACCES), or other problems GitProcess.exec and GitProcess.spawn would throw an error due to the stdio streams on the launched child process not being available. This error would mask the actual, underlying error which would be emitted asynchronously and since we hadn't subscribed to the error handler of the process yet (due to the masking exception) the exception would be raised as an unhandled exception and the app would crash.

In addition to fixing this "unpreventable" crash this PR audits the places in Desktop where we assume that the process.std* streams are always present and adds som extra debugging data for us to use instead of the generic spawn /Users/.../git: EAGAIN error thrown by Node.

Release notes

Notes: [Fix] Prevent application crash when system is unable to launch Git

@niik niik added this to In Progress PRs in Desktop 2.2.4 release via automation Nov 11, 2019
@niik niik moved this from In Progress PRs to Awaiting Review in Desktop 2.2.4 release Nov 11, 2019
@outofambit outofambit self-requested a review November 12, 2019 15:41
outofambit
outofambit previously approved these changes Nov 14, 2019
Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

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

one concern about dugite versioning, but its not blocking. looks good to me!

app/package.json Outdated Show resolved Hide resolved
app/src/ui/dispatcher/error-handlers.ts Show resolved Hide resolved
@outofambit outofambit self-assigned this Nov 14, 2019
Co-Authored-By: evelyn masso <outofambit@github.com>
Desktop 2.2.4 release automation moved this from Awaiting Review to Review In Progress Nov 14, 2019
@niik niik requested a review from outofambit November 14, 2019 18:23
outofambit
outofambit previously approved these changes Nov 14, 2019
@outofambit
Copy link
Contributor

CI is failing here because we didn't update the lockfile with package.json. pushing that tiny change up now so build can go green and i can merge.

@outofambit outofambit merged commit bf9a01c into development Nov 14, 2019
Desktop 2.2.4 release automation moved this from Review In Progress to PRs For Next Beta Nov 14, 2019
@outofambit outofambit deleted the spawn-crash-debug branch November 14, 2019 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Desktop 2.2.4 release
PRs For Next Beta
Development

Successfully merging this pull request may close these issues.

None yet

2 participants