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

Implement register and flag::register for Windows. #19

Merged
merged 6 commits into from Jul 26, 2019

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Jul 21, 2019

This is another attempt at Windows support in #18.

Main differences from #18:

  • pipe/iterator supports are postponed.
  • Not using SetConsoleCtrlHandler directly. Instead, use signal/raise directly.
  • We no longer have the __emulate_kill workaround. raise works.

As for #18, confirmed to catch Ctrl-C in some cases, but not all cases.

Copy link
Owner

@vorner vorner left a 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.
Copy link
Owner

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“.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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 global handler 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.

signal-hook-registry/src/lib.rs Show resolved Hide resolved
signal-hook-registry/src/lib.rs Outdated Show resolved Hide resolved
signal-hook-registry/src/lib.rs Outdated Show resolved Hide resolved
signal-hook-registry/src/lib.rs Outdated Show resolved Hide resolved
src/flag.rs Show resolved Hide resolved
src/flag.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/iterator.rs Show resolved Hide resolved
tests/tokio.rs Show resolved Hide resolved
@vorner
Copy link
Owner

vorner commented Jul 25, 2019

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.

@vorner vorner merged commit ea6182a into vorner:master Jul 26, 2019
@vorner
Copy link
Owner

vorner commented Jul 26, 2019

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.

@vorner
Copy link
Owner

vorner commented Jul 27, 2019

Looking through the newly introduced API, there seems to be nearly nothing (only the register_signal_unchecked function), so I think it's fine to promise API stability on this.

Do you want to add yourself to the crate authors, before I release?

@qnighy
Copy link
Contributor Author

qnighy commented Jul 27, 2019

Do you want to add yourself to the crate authors, before I release?

Yes, I'll prepare a PR in minutes...

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

Successfully merging this pull request may close these issues.

None yet

2 participants