Skip to content
New issue

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

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add support for detecting window close events (on Windows) #6808

Closed
wants to merge 1 commit into from

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Dec 27, 2019

WIP fix for #1897

We add a shim nativeNotifier who's responsibility is to notify a channel of events/signals not covered by the signal pkg.
In this case, we implement this to add support for Windows' window events.

When the window receives a CTRL+C, CTRL+BREAK, or WM_CLOSE event, we will notify the interrupt channel and tell the OS we proccessed the event successfully.
This in turn will cancel the node's context for the first event and exit outright upon subsequent interrupts.

Current problem:
When processing CTRL_C_EVENT and CTRL_BREAK_EVENT events, nothing special has to be considered. But WM_CLOSE breaks down to a CTRL_CLOSE_EVENT which has special implications in this context.
https://docs.microsoft.com/en-us/windows/console/handlerroutine

When we receive this message, the system will terminate us in either 5 seconds, or when we return from our event handler (whichever is first).
Meaning we can initiate graceful shutdown from any signal, but need a way to wait for shutdown to be completed before returning from the handler, specifically when handling the CTRL_CLOSE_EVENT event.

i.e. the node has to...

case windows.CTRL_CLOSE_EVENT:
	// start shutting down here
	...
	// and block here until shutdown is finished
	return processed // this is the moment the process dies 👀

Commands like shutdown have direct access to the node and can just call its Close method, blocking until the node is finished shutting down.
We're a background thread that gets called at some arbitrary point by the user/OS, and don't have access to the node object so we can't just do the same.

This needs to be taken care of before this can progress out of a draft.

Edit:
video showing the signal getting caught

Allows for a potentially more graceful shutdown compared to immediate termination
case windows.CTRL_CLOSE_EVENT:
//NOTE: the OS will terminate our process after receiving this event
// either after `SPI_GETHUNGAPPTIMEOUT` or immediately after we return non-0
// (whichever is first)
Copy link
Contributor Author

@djdv djdv Dec 27, 2019

Choose a reason for hiding this comment

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

This is 5AM influenza English, look at again later.
Edit: applies to all comments
Edit2: and the code too

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks!

// call it so it can inject signals from the OS
if nativeNotifier != nil {
nativeNotifier(ih, notify, sigs...)
}
Copy link
Member

Choose a reason for hiding this comment

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

This should probably go below the ih.wg.Add call (or maybe just move that goroutine to the top of the function?). Otherwise, we could:

  1. Receive a window close event before we call ih.wg.Add
  2. Call ih.wg.Wait()
  3. Succeed.
  4. Either the process gets killed or we then call ih.wg.Add(1) and panic (because we've already called wait on the waitgroup.

if !goIsHandlingIntterupts {
sigChan <- os.Interrupt
}
return processed
Copy link
Member

Choose a reason for hiding this comment

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

If go is handling interrupts, will this cause problems? I wonder if we should just always use this function on windows (skipping the signal.Notify call).

@djdv
Copy link
Contributor Author

djdv commented Jan 16, 2020

Note:
Conveniently timed, Go 1.14 will handle this directly.
golang/go@5d1a951

Will need to see how this behaves in 1.14 and hide this patch behind a build constant for <=1.13.
This way we handle the signal ourselves on old versions of the compiler, but don't do anything special on 1.14+.
(It's likely still preferable to catch and handle the signal ourselves even on 1.14 though, will look into)

@Stebalien
Copy link
Member

1.14 is out

@djdv
Copy link
Contributor Author

djdv commented Mar 11, 2020

Since Go 1.14 handles this natively we can just drop this hack.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants