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

os: Interrupt is not sendable on Windows #6720

Closed
dsymonds opened this issue Nov 6, 2013 · 28 comments
Closed

os: Interrupt is not sendable on Windows #6720

dsymonds opened this issue Nov 6, 2013 · 28 comments

Comments

@dsymonds
Copy link
Member

@dsymonds dsymonds commented Nov 6, 2013

Sending os.Interrupt via os.Process.Signal works on most platforms, but doesn't work on
Windows. There is a TODO(rsc) to do it. Someone should do it for Go 1.3:

https://code.google.com/p/go/source/browse/src/pkg/os/exec_windows.go?name=go1.2rc3#62

func (p *Process) signal(sig Signal) error {
        if p.done() {
                return errors.New("os: process already finished")
        }
        if sig == Kill {
                return terminateProcess(p.Pid, 1)
        }
        // TODO(rsc): Handle Interrupt too?
        return syscall.Errno(syscall.EWINDOWS)
}
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 6, 2013

Comment 1:

There is no such thing as "signal" on windows. You cannot "send a signal to a process".
You can only kill process -
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx. If you
use TerminateProcess API, process that gets killed has no knowledge of the event - it
just dies.
I don't see how your request can be implemented. Tell me more about your problem. Thank
you.
Alex
@okdave

This comment has been minimized.

Copy link
Contributor

@okdave okdave commented Nov 6, 2013

Comment 2:

My understanding is that equivalent is the CTRL_C_EVENT. As far as I can tell, these
events are already received and interpreted correctly in os.Signal, its just not
possible to send them.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 6, 2013

Comment 3:

Oh yes, we can do that. One of our tests in os/signal already does that. So we'll just
have to move that bit of code into the package. I will so that, if you like.
Alex
@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Nov 6, 2013

Comment 4:

Please do, once Go 1.2 is out the door and the tree is re-opened.
@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Nov 6, 2013

Comment 5:

Owner changed to @alexbrainman.

@dvyukov

This comment has been minimized.

Copy link
Member

@dvyukov dvyukov commented Nov 9, 2013

Comment 6:

Windows does have limited support for signals within subsystem for Unix based
applications. the implementation is user space and is based on
SuspendThread/SetThreadContext hackery.
If that's useful Go can do the same - support intra/inter-process signals using some
private interface.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 9, 2013

Comment 7:

We use win32 API only. I would like to stick to it. Also I have used
SuspendThread/SetThreadContext hackery recently in profile collector and it behaves
unpredictably. David how are you going to use this facility? What exactly do you need?
Alex
@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Nov 9, 2013

Comment 8:

The use case is process management. I want to be able to send a signal to another
process to tell it to shut itself down gracefully. Its exact implementation on Windows
is irrelevant to my use case as long as proc.Signal(os.Interrupt) causes that value to
appear via signal.Notify(c, os.Interrupt) in the other process.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 9, 2013

Comment 9:

Go command does just that by handling CTRL_C_EVENT, so I think we are good.
Alex
@okdave

This comment has been minimized.

Copy link
Contributor

@okdave okdave commented Nov 13, 2013

Comment 10:

It seems that CTRL_C_EVENTS are handled correctly. You can register for os.Interrupt
with signal.Notify and this signal will be delivered when you press CTRL+C in the
command prompt.
However, it doesn't seem possible to send these events from a program:
process.Signal(os.Interrupt) doesn't have any effect on the process being signalled.
If it's not possible to achieve this in win32, maybe we need to settle for clarifying
the language in http://golang.org/pkg/os/#Signal which implies os.Interrupt and os.Kill
will work everywhere.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 13, 2013

Comment 11:

djd,
I plan to implement process.Signal(os.Interrupt) on windows. In fact we already have
something like it already here
https://code.google.com/p/go/source/browse/src/pkg/os/signal/signal_windows_test.go#17.
After Go 1.2 is released.
Alex
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2013

Comment 12:

It will be good to get os.Interrupt working.
The magic described by Dmitriy is not something we should do. Go is
supposed to work well with the rest of the system. If the rest of the
system does not support other signals, Go should not invent them for use by
Go programs alone.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 19, 2013

Comment 13:

David,
Life is never simple. I have a problem (I remember now I was fighting with that test
back then too).
I am using GenerateConsoleCtrlEvent Windows API
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms683155(v=vs.85).aspx). It
sends signal to a "console process group". The "console" here is important, I think - it
is not about process here, it is about console that group of processes share between
them.
New process can be started with CREATE_NEW_PROCESS_GROUP flag - from what I can gather,
it makes new process use new "console", otherwise it shares it with the parent. If
process is started with CREATE_NEW_PROCESS_GROUP flag, it also means new "process group"
is created and that group can be referred to by new process id.
The GenerateConsoleCtrlEvent has 2 arguments: dwCtrlEvent and dwProcessGroupId. We can
supply new pid in dwProcessGroupId, but it will only work, if new process is started
with CREATE_NEW_PROCESS_GROUP flag. Alternatively, dwProcessGroupId has to be 0, then
the signal is sent to "current process group" - that will send signal to the parent as
well as client (it will go to everyone who shares current console).
dwCtrlEvent can be CTRL_C_EVENT or CTRL_BREAK_EVENT, but according to the doco
CTRL_C_EVENT cannot be used unless dwProcessGroupId is 0.
So, the only way to use GenerateConsoleCtrlEvent is with dwCtrlEvent=CTRL_BREAK_EVENT
and dwProcessGroupId set to a client process pid that was started with
CREATE_NEW_PROCESS_GROUP flag. That is what my test does. Will that suit you?
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 19, 2013

Comment 14:

Here https://golang.org/cl/29290044, if someone wants to try it.
Alex
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 15:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 16:

Labels changed: added repo-main.

@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Dec 4, 2013

Comment 17:

Sounds like it's in progress.

Labels changed: added release-go1.3, removed release-none.

Status changed to Started.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 18:

David, do you have any comments on my comments in #13 and #14?
Alex
@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Dec 4, 2013

Comment 19:

I think that the exact mechanism largely doesn't matter, as long as if you are using
os/signal.Notify on the receiver process that it'll receive the os.Interrupt.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 4, 2013

Comment 20:

I think there is no "general" solution to your request. 
Ctrl-break is sent to "console process group" - all processes in the group get signal.
There is no way to send signal to an individual process.
There are more complications, but even this first one is unacceptable. Isn't it?
Alex
@dsymonds

This comment has been minimized.

Copy link
Member Author

@dsymonds dsymonds commented Dec 4, 2013

Comment 21:

That is a pain, yes.
@okdave

This comment has been minimized.

Copy link
Contributor

@okdave okdave commented Dec 4, 2013

Comment 22:

From my understanding, making a new console process group also messes up other things.
For example, the processes no longer listen to CTRL-C from the console the user invoked
the original/parent process in.
Maybe the right course here is to document this limitation more clearly: Interrupt
doesn't work cleanly in every os.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 5, 2013

Comment 23:

> ... For example, the processes no longer listen to CTRL-C from the console the user
invoked the original/parent process in.
Correct.
> ... Maybe the right course here is to document this limitation more clearly: Interrupt
doesn't work cleanly in every os.
Sounds fine to me. Should I drop my change then?
Alex
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 3, 2014

Comment 24:

I'll leave this open in case someone wants to try it later, but I don't think there's
much to do here. David, let this be a lesson to you to check return values.

Labels changed: added suggested, removed priority-soon, size-m, release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 3, 2014

Comment 25:

Labels changed: added release-none.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented May 14, 2014

Comment 26:

The doco: https://golang.org/cl/92340043/
Alex
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 14, 2014

Comment 27:

CL https://golang.org/cl/92340043 mentions this issue.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented May 23, 2014

Comment 28:

This issue was closed by revision 05cc78d.

Status changed to Fixed.

@dsymonds dsymonds added fixed labels May 23, 2014
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
Fixes golang#6720.

LGTM=bradfitz
R=golang-codereviews, iant, bradfitz
CC=golang-codereviews
https://golang.org/cl/92340043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.