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

Is Windows CTRL_CLOSE correctly handled? #94

Open
Astlaan opened this issue Dec 15, 2024 · 13 comments
Open

Is Windows CTRL_CLOSE correctly handled? #94

Astlaan opened this issue Dec 15, 2024 · 13 comments

Comments

@Astlaan
Copy link

Astlaan commented Dec 15, 2024

As far as I know, in Windows, handling the CTRL_C event is easier, since it can be listened to and then handled in an asynchronous manner by the program.

But from what I have seen, this seems not to work for CTRL_CLOSE signals. This is because, when this signal is received by a windows console app, the thread of the running program is paused (although the rust thread might still have time to catch the signal and run a small piece of code), and the registered CtrlHandler function is called in a new thread. But whatever code rust can still run before its thread is paused will not be well defined behaviour, since it is dependent on the system latency to pause the thread.

On the other hand, windows allow functions defined by the SetConsoleCtrlHandler entity to run up to 5 seconds before they are forcefully terminated. The rust thread, however, will just have whatever latency the os has in pausing the thread, which is naturally much shorter.

I.e. the mechanism of the the CTRL_CLOSE (also CTRL_SHUTDOWN and maybe others) signal in windows is much different from the CTRL_C. Whereas the SIGINT/SIGTERM/SIGHUP in UNIX all work in a similar way amongst themselves and can be asynchronously handled in the rust main thread.

I can be wrong, but this was my understanding of the topic.

Check this example in the rust-ctrlc crate:
https://github.com/Detegr/rust-ctrlc/blob/master/src/platform/windows/mod.rs

Maybe this Go-related github issue can also be useful, it is where I learned about the issue:
golang/go#41884 (comment)

(Also, while in Linux all the applications run in the same subsystem, in Windows, apps can run be of the Console or GUI types, and they run in different subsystems. By default Rust compiles as Console app, which opens a terminal while running. But from what I can gather, GUI apps receive different signals than those in Console. For example, WM_CLOSE instead of CTRL_CLOSE. I think these can be handled the same way as CTRL_C. Maybe it could be interesting to support GUI-app signals as well.)

@Finomnis
Copy link
Owner

I'm not entirely sure if I understand what should be done different... What exactly would be the scenario where the current code does not work, and what effect would that have?

@Finomnis
Copy link
Owner

GUI Apps should handle the WM_CLOSE signal in the GUI code, I don't think this crate would be the correct place to deal with this.

@Astlaan
Copy link
Author

Astlaan commented Dec 15, 2024

What I mean is that I think that the CTRL_CLOSE signal handling in this app won't grant any time for a graceful shutdown to happen. tokio might have time to catch the signal, but the thread may be frozen shortly after.

Instead, unlike the CTRL_C methodology, for CTRL_CLOSE events you might have to implement something similar to what was done in that file from the rust-ctrlc crate I mentioned above. I.e. register a function to be run on CTRL_CLOSE, and it is registered with the SetConsoleCtrlHandler as demonstrated in that file.

@Finomnis
Copy link
Owner

I'm not sure what you mean with the "similar to the rust-ctrlc crate".

If it is true that only the function defined in SetConsoleCtrlHandler runs and everything else will be paused indefinitely, then there is nothing we can do. This is for async systems, and if the async reactor stops running, then we cannot work around that, unless I misunderstand something here.

@Astlaan
Copy link
Author

Astlaan commented Dec 15, 2024

#[cfg(windows)]
let mut signal = tokio::signal::windows::ctrl_close().expect("Failed to create signal handler");

let ctrl_c = tokio::signal::ctrl_c();

// Keep the program running and handle signals
tokio::select! {
    _ =
        signal.recv()
     => {
        let _ = File::create("empty_async_signal.txt");
        cleanup_ports(&gateway, external_port);
    }
    _ = ctrl_c => {
        let _ = File::create("empty_async_ctrlc.txt");
        cleanup_ports(&gateway, external_port);
    }
}

As an example, I initially tried to run this section of code. In case of ctrl_c, the whole code is run and the program shuts down properly. but in the ctrl_close case, only the file creation was executed, and not the cleanup_ports function. If I commented the file creation line, then cleanup_ports executes but only partially, not fully (probably because the thread is frozen before it has time to finish).

@Finomnis
Copy link
Owner

Finomnis commented Dec 15, 2024

Again, if this is how it works, I'm not sure how to handle it correctly. This mechanism of windows might simply be incompatible with async programs then.

@Astlaan
Copy link
Author

Astlaan commented Dec 15, 2024

I created a small repo to test this:
https://github.com/Astlaan/rust-ctrl-close-handler/blob/main/src/main.rs

It seems I was wrong. The main rust thread is not frozen when the handler function is called.

To test it, you have to do cargo build and then double click on the executable on windows (not sure if that's your OS?)
The main thread saves a file every second. Then click on the X button to close, and you can see that not only the handler starts saving the files (prefixed by m_), but the main thread also doesn't stop saving them.

So it seems the async app can still cleanup after itself directly. I think you only have to set up a sleep in the handler, so that the handler doesn't return before the time is up. Or, more correctly, some sort of flag to that the handler thread knows when it is time to return.

Ofc, it should be clear to the user that the app only has 5 seconds to clean itself before being forcefully terminated.

As for the Gui apps, they can be fully without using this console handler. I.e. using the same method you used so far. Should be easy to extend support, probably you only have to add listening for signals relative to GUI apps.

@Finomnis
Copy link
Owner

Finomnis commented Dec 16, 2024

I'm unsure what you mean with "set up a sleep in the handler". Which handler are you referring to?

I mean, I get what you are talking about; you need to prevent the handler thread from returning, because Windows interprets that as "I am done cleaning up, you can kill now".

But this crate does not directly provide the signal handler. What you are reporting here might be a tokio problem.

@Finomnis
Copy link
Owner

Finomnis commented Dec 16, 2024

For reference, the function that you are referring to is probably this one:

https://github.com/tokio-rs/tokio/blob/10e23d1c621ab38aadf2cefba1120494cff615f0/tokio/src/signal/windows/sys.rs#L109

It gets registered to SetConsoleCtrlHandler here:
https://github.com/tokio-rs/tokio/blob/10e23d1c621ab38aadf2cefba1120494cff615f0/tokio/src/signal/windows/sys.rs#L96

I don't have any control over this function, this is deeply embedded in tokio itself.

I'm not entirely sure if the tokio team is aware of this; it might be worth asking on the tokio discord or the tokio issue tracker.

From what I can see, tokio's handler sets a signal and returns immediately. So it does sound like a tokio problem.

@Finomnis
Copy link
Owner

Finomnis commented Dec 16, 2024

That said, do you think this change was a mistake from my side?

3ac73fe#diff-6c2f9810ae3295fc04958cc626d918ac033edf18290ed77858201ab570c538d8R34

Should I revert CTRL_CLOSE and CTRL_SHUTDOWN?

I don't think I can properly write code that handles those two; the only option left might be to not handle them at all and have the user of this crate take control over this himself. Although that would probably collide with tokio's implementation; you might be correct that this problem needs to be escalated to the tokio devs.

@Astlaan
Copy link
Author

Astlaan commented Dec 16, 2024

Lets see. In the tokio file that you sent, it seems that the handler function simply registers an event and then returns right away. Which would cause the program to be shut down.

unsafe extern "system" fn handler(ty: u32) -> BOOL {
    let globals = globals();
    globals.record_event(ty as EventId);

    // According to https://docs.microsoft.com/en-us/windows/console/handlerroutine
    // the handler routine is always invoked in a new thread, thus we don't
    // have the same restrictions as in Unix signal handlers, meaning we can
    // go ahead and perform the broadcast here.
    if globals.broadcast() {
        1
    } else {
        // No one is listening for this notification any more
        // let the OS fire the next (possibly the default) handler.
        0
    }
}

According to:
https://learn.microsoft.com/en-us/windows/console/handlerroutine

It seems that if 1 is passed, it means that the handler handled the shutdown, and it is ready to be shutdown. If 0 is passed, "next handler function in the list of handlers for this process is used". Also from the https://learn.microsoft.com/en-us/windows/console/setconsolectrlhandler :

When a console process receives any of the control signals, its handler functions are called on a last-registered, first-called basis until one of the handlers returns TRUE. If none of the handlers returns TRUE, the default handler is called.

But in the function, they call broadcast() which "Returns true if an event was delivered to at least one listener". It seems that the way tokio implements it doesn't even give a change for the program to do anything with the signal, since it broadcasts it to the listeners, and if they exist, the handler function returns a true, which would terminate the program right away?


PS: Apparently I also wasn't able to get it to work with the ctrlc crate.
Detegr/rust-ctrlc#122 (comment)

@Finomnis
Copy link
Owner

Not sure how to respond now; if you suspect that this is a Tokio issue, your should report it there. I can't really change it here.

@Astlaan
Copy link
Author

Astlaan commented Dec 16, 2024

I just created an issue there indeed:
tokio-rs/tokio#7039

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

No branches or pull requests

2 participants