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

block signals during forking #218

Merged
merged 9 commits into from
Apr 7, 2020
Merged

block signals during forking #218

merged 9 commits into from
Apr 7, 2020

Conversation

jerch
Copy link
Collaborator

@jerch jerch commented Aug 27, 2018

WIP - do not merge (yet)

This is a first version to deal with the signal problem during spawning the pty and forking. Not tested yet due to missing test cases. (some test cases added).

@msftclas
Copy link

msftclas commented Aug 27, 2018

CLA assistant check
All CLA requirements met.

@jerch jerch mentioned this pull request Aug 27, 2018
@fpronto
Copy link

fpronto commented Aug 28, 2018

good for me

@jerch
Copy link
Collaborator Author

jerch commented Aug 28, 2018

The tests cover:

  • killing with SIGHUP, which works now but didnt before for some unknown reason
  • some custom signal handler for SIGUSR1 will stay in place for parent and child
  • previously attached SIGINT handlers work as expected in parent and child

Tested on Ubuntu 14 & 16, OpenIndiana (Solaris) and FreeBSD 11.0.

Up for review and tests on OSX.

@Tyriar Tyriar added this to the 0.7.7 milestone Aug 29, 2018
@Tyriar Tyriar self-assigned this Aug 29, 2018
@Tyriar Tyriar modified the milestones: 0.7.7, 0.7.8 Sep 5, 2018
@olivierchatry
Copy link

Very nice ! Would be very cool to have that, as it is a bit of a problem when you have clean up to do in your "main" process if user interrupt it.

@Tyriar Tyriar modified the milestones: 0.7.8, 0.7.9 Sep 28, 2018
@Tyriar Tyriar closed this Dec 21, 2018
@Tyriar Tyriar reopened this Dec 21, 2018
@Tyriar Tyriar modified the milestones: 0.7.9, 0.8.1 Dec 21, 2018
@olivierchatry
Copy link

Just checking if this will merge soon. Sorry for being pushy, but it is kind of important for my project.

@Jesse-Schokker
Copy link

This is quite a serious bug. Can someone merge it in already please?

@Tyriar Tyriar modified the milestones: 0.8.1, 0.8.2 May 2, 2019
d4tocchini added a commit to d4tocchini/node-pty that referenced this pull request Sep 15, 2019
@Tyriar Tyriar removed this from the 0.9.0 milestone Oct 22, 2019
@gae123
Copy link

gae123 commented Feb 13, 2020

We really need this fix, what is blocking its integration?

@Tyriar Tyriar added this to the 0.10.0 milestone Apr 7, 2020
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

@jerch I don't really understand the change in pty.cc but if you think it's good and low risk for VS Code then I'm fine to merge it in.

@Tyriar Tyriar linked an issue Apr 7, 2020 that may be closed by this pull request
@jerch
Copy link
Collaborator Author

jerch commented Apr 7, 2020

@Tyriar Well the changes do the following:

  • temporarily block signals during forking (to avoid a race condition)
  • set signal handlers in child to default handler (to avoid triggering parent handlers in the child between fork --> exec)
  • re-enable signals:
    • parent: restore prev handlers
    • child: execs into foreign binary with default signal handlers, thus the binary behaves as expected from a clean start
  • remove resetting of SIGINT handler, which causes most issues listed

I dont expect negative side effects on vscode here, but cannot tell that for sure. There is a small chance, that some code might rely on the faulty handling of the old code.

I think removing the race conditions makes things more stable (its just an edge case, thus hardly triggered/seen by anyone, but I think that the old code could segfault the process if triggered). And not resetting the SIGINT handler in the parent should fix the issues most ppl have with the old code.

So yes I think this is good to go. You still might want to check in vscode, if spawning/closing pty processes triggers weird behavior afterwards like dysfunctional process.on('some_signal') handlers. I dont think so.

@Tyriar
Copy link
Member

Tyriar commented Apr 7, 2020

Merging and will see if there are problems in the first vscode 1.45 insiders build

@Tyriar Tyriar merged commit 59e06f9 into microsoft:master Apr 7, 2020
Tyriar added a commit to microsoft/vscode that referenced this pull request Apr 7, 2020
@NuSkooler
Copy link
Contributor

Any update on getting this into a release?

@Tyriar
Copy link
Member

Tyriar commented Jan 20, 2021

@NuSkooler you can use the beta tag to get the latest version, no date for 0.10 yet

lemanschik pushed a commit to code-oss-dev/code that referenced this pull request Nov 25, 2022
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.

process.on('exit') is disabled after spawn a child process.
8 participants