-
Notifications
You must be signed in to change notification settings - Fork 78
Implement register and flag::register for Windows. #19
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, I like it :-).
I never learned not to nitpick, so there are few suggestions about naming or comments. Nothing of any particular substance, though.
//! | ||
//! - Due to lack of `siginfo_t`, we don't provide `register_sigaction` or `register_unchecked`. | ||
//! - Due to lack of signal blocking, there's a race condition. | ||
//! After the call to `signal`, there's a moment where we miss a signal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest listing the consequences here ‒ „If this happens, the default handler is called instead“.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
signal()
comes before RCU store -- so I guess nothing happens (neither the default handler nor the registered handler is called) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand. Or, are we both thinking about the same race condition?
This is what I fear might happen:
- SIGTERM (or something else) arrives. This resets the signal to default, which is terminate.
- It calls the handler.
- Before the handler can re-establish itself back from the default, another SIGTERM arrives. This one terminates the program, because it didn't get re-established.
It is probably quite rare for the user to press CTRL+C that fast, so it most probably won't happen, but in theory something could send them programmatically?
But I'm not sure about the theory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I imagined a different one than yours, so I added another bullet here to describe yours. The one in my mind was:
- The program tries to register a handler for SIGINT. It acquires the RCU lock. We don't block signals here because there's no such machinery in Windows.
signal
is called. Our globalhandler
function is registered.- SIGINT arrives. As
handler
is already registered, the default handler isn't invoked. However, we haven't stored the RCU result yet so there's no shared handler registered. - RCU store happens.
In this case we miss SIGINT where we expect the default handler ("before handler registration" case) or the registered handler ("after handler registration" case) to be invoked.
Getting there :-). There are these little details I'm not sure about I'd like you to have a look at, but if you think I'm wrong, it'd be fine to merge it as it is. I'm thinking about one other thing. This sub-crate is already 1.0. Adding new functions is fine, but that means promising not to ever remove or change their signatures. I'll probably ask around a bit, if there's a convention how to get an unstable part of API in otherwise stable crate ‒ so maybe I'll hold releasing it a bit after it gets merged, to be on the safe side. I hope it's not a problem. If you have a suggestion how to solve this, I'd be interested in hearing it. |
Thank you. I'll mark the new functions/the ones that have different signature in some way to clearly state they may still change a bit and will release the registry crate over the weekend. |
Looking through the newly introduced API, there seems to be nearly nothing (only the Do you want to add yourself to the crate authors, before I release? |
Yes, I'll prepare a PR in minutes... |
This is another attempt at Windows support in #18.
Main differences from #18:
pipe
/iterator
supports are postponed.SetConsoleCtrlHandler
directly. Instead, usesignal
/raise
directly.__emulate_kill
workaround.raise
works.As for #18, confirmed to catch Ctrl-C in some cases, but not all cases.