block signals during forking

[jerch]

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]

All CLA requirements met.

[fpronto]

good for me

[jerch]

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.

[olivierchatry]

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.

[olivierchatry]

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

[Jesse-Schokker]

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

[gae123]

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

[Tyriar]

@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.

[jerch]

@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]

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

[NuSkooler]

Any update on getting this into a release?

[Tyriar]

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