chocolatey / home

The place to start for issues with areas of Chocolatey that are infrastructure related, or really any issues could be started here. There is also choco for the CLI client, Chocolatey GUI for the GUI.
Apache License 2.0
29 stars 10 forks source link

On Control+C, send interrupt signal first before killing process #134

Open ferventcoder opened 7 years ago

ferventcoder commented 7 years ago

Give the process a little time to handle Control+C before nuking it.

┆Issue is synchronized with this Gitlab issue by Unito

TheOneRing commented 7 years ago

or don't kill it at all, which would be the expected behaviour for a wrapper

improbablepiotr commented 7 years ago

Hi All, Are there any plans to address this issue? I think the wrapper should have no logic at all and simply pass the signals down to the process. Our applications are now unable to execute their clean-up logic in the shutdown hook, because they are being killed by the shim.

ferventcoder commented 7 years ago

@TheOneRing I'm not sure that would be best, if someone is trying to kill the process, they would hit control+c. For the most part, it's a shim, which is supposed to behave as if they were calling the actual, so whatever the actual processes behavior for Control+C should be applied.

ferventcoder commented 7 years ago

It's a darned if you do, darned if you do not kind of situation - we used to pass the sigint down, and folks complained that it would just sit there on some processes, which lead us to send kill.

How about this? We start with sigint on the first Control+C, then send sigkill on the next Control+C if we receive it. It seems to meet both scenarios and can catch hanging shims that are waiting on the process to clean up and close.

improbablepiotr commented 7 years ago

+1 sounds good to me.

TheOneRing commented 7 years ago

Hm I'd prefer if console applications won't be killed by the shim. For example I wrapped calls to interactive interpreters, python, msys etc . But I probably could go back to bat scripts in that case.

gerardog commented 4 years ago

IMHO It's better that the wrapper SHIM simply ignores/forward the signal to the child process.

ferventcoder commented 4 years ago

When using Choco-Shim for gsudo: pressing CTRL-C kills the shim but keeps gsudo open.

@gerardog I read through the issue you linked here and I'm a bit confused - Ctrl+C sends sigkill to the underlying process, so it would/should have killed gsudo. I think there is maybe something else going on related to how gsudo works and the shim doesn't play nice with that.

gerardog commented 4 years ago

Think of it this way: You have a Pwsh/Cmd shell and you run cmd /c PING localhost -t, which will loop forever. When you press CTRL-C, the inner PING.EXE ends gracefully and return some stats. On the other hand, the wrappee 'CMD /c' is not killed, it justs ends after PING finalizes. The same applies if gsudo (without shim) is in the chain. But the moment you introduce the choco shim in the middle (as if you shim gsudo), then the Ctrl-C behaviour is altered: the shim dies, but PING does not. Even killing the PING process is not the same as the gracefull, native, Ctrl-C handling.

Try to shim CMD or PING and you will see the same effect ocurring to gsudo.

Makes sense?

gerardog commented 4 years ago

You may think of gsudo as a "shim" (of either cmd or the process referenced by the input arguments) that also elevates permissions. If Choco Shim is also C#, I think the exact same behaviour should work for all scenarios. (which is, delegate all CTRL-C handling to the shimmed process).

See https://github.com/gerardog/gsudo/blob/d1b4abb3d069aaa1b5f06e6f9ecb7a1a83f689a4/src/gsudo/ProcessRenderers/TokenSwitchRenderer.cs#L61 https://github.com/gerardog/gsudo/blob/d1b4abb3d069aaa1b5f06e6f9ecb7a1a83f689a4/src/gsudo/Helpers/ConsoleHelper.cs#L33 https://github.com/gerardog/gsudo/blob/d1b4abb3d069aaa1b5f06e6f9ecb7a1a83f689a4/src/gsudo/Native/ConsoleApi.cs#L41

ferventcoder commented 4 years ago

@gerardog It's c# - this is basically what it looks like: https://github.com/chocolatey/shimgen/blob/8f549bfbfa915fdfb556b998a0cf25c97eca4b78/shim/ShimProgram.cs#L262-L311

gep13 commented 2 years ago

Based on some changes happening to the Chocolatey repositories, this issue has been moved from the chocolatey/shimgen repository to the chocolatey/home repository.