Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Change the behaviour of core::run::Program.destroy to forcibly terminate the program #5761

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

Dretch commented Apr 6, 2013

As proposed in issue #5632.

I added some new stuff to libc - hopefully correctly. I only added a single signal constant (SIGKILL) because adding more seems complicated by differences between platforms - and since it is not required for issue #5632 then I figure that I can use a further pull request to flesh out the SIG* constants more.

gareth added some commits Mar 31, 2013

gareth Fix a bug where calling p.destroy() on the result of calling
start_program(...) would cause a segfault when p went
out of scope due to out_file/err_file being closed twice.
e081c17
gareth Update doc-comments to reflect the current year and trait
names now being capitalized.
622bb63
gareth Change the behaviour of core::run::Program.destroy to
forcibly terminate the program (as suggested in issue #5632)
483e95a
Contributor

thestinger commented Apr 6, 2013

Perhaps this should use SIGTERM, with a more dangerous naming for SIGKILL (force_destroy?). I don't think SIGKILL is usually what you want.

Contributor

Dretch commented Apr 7, 2013

@thestinger

That sounds reasonable. There is the question of how the SIGTERM equivalent should work on Windows, though - it looks like a can of worms to implement such a thing, and it only works for GUI apps (http://stackoverflow.com/questions/2055753/how-to-gracefully-terminate-a-process).

I suppose the Windows equivalent of the SIGTERM sender could either do nothing or alternatively just call TerminateProcess like the the SIGKILL equivalent does. It might be safer if it did nothing because on Unix SIGTERM may well be ignored by the target process.

gareth Make destroy() send SIGTERM and add a new method called
force_destroy() that sends SIGKILL - as suggested by 
@thestinger.
995d444
Contributor

Dretch commented Apr 11, 2013

@thestinger I have added a new commit with the changes you suggested.

Both destroy() and force_destroy() have identical behaviour on win32 (they both call TerminateProcess()) because on balance I think that behaviour is more predictable than destroy() simply not doing anything on win32.

r+

Contributor

bors replied Apr 13, 2013

saw approval from thestinger
at Dretch/rust@995d444

Contributor

bors replied Apr 13, 2013

merging Dretch/rust/murder-death-kill = 995d444 into auto

Contributor

bors replied Apr 13, 2013

Dretch/rust/murder-death-kill = 995d444 merged ok, testing candidate = e2d5ceb

Contributor

bors replied Apr 13, 2013

fast-forwarding incoming to auto = e2d5ceb

@bors bors added a commit that referenced this pull request Apr 13, 2013

@bors bors auto merge of #5761 : Dretch/rust/murder-death-kill, r=thestinger
As proposed in issue #5632.

I added some new stuff to libc - hopefully correctly. I only added a single signal constant (SIGKILL) because adding more seems complicated by differences between platforms - and since it is not required for issue #5632 then I figure that I can use a further pull request to flesh out the SIG* constants more.
e2d5ceb

@bors bors closed this Apr 13, 2013

@bors bors added a commit that referenced this pull request Apr 14, 2013

@bors bors auto merge of #5880 : Dretch/rust/signals, r=thestinger
This is a follow-up to #5761. Its purpose is to make core::libc more consistent - it currently only defines SIGKILL and SIGTERM, because they are the only values that happen to be needed by libcore.

This adds all the posix signal value constants, except for those that have different values on different architectures.

The output of the command `man 7 signal` was used to compile these signal values.
1ab1354
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment