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

Add on_log_quit. #2669

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add on_log_quit. #2669

wants to merge 1 commit into from

Conversation

guilt
Copy link

@guilt guilt commented Feb 17, 2021

Request for comments;

Closes #2590

I was unable to find a working at_exit that is publicly available in stable rust today. So I decided to post this and ask what may be the right way to attempt this?

@kinnison kinnison marked this pull request as draft February 20, 2021 10:46
@kinnison
Copy link
Contributor

Does this demonstrably fix the issue in question? It's relying on a feature flag which means we won't accept it into Rustup because we build with stable for releases, but it could point the way to warranting investigation into a way to resolve this in a stable release if you are certain it functions.

In the meantime, since this is not mergeable as-is regardless of correctness, I have marked the PR as a draft

@guilt
Copy link
Author

guilt commented Feb 20, 2021

Thanks!

I will generally wait for the at_exit to get merged to stable/beta at this point. This isn't a priority for 1.24.x, but the reset handler would have to be called somewhere.

To reproduce, I would basically introduce some artificial delays in the log code, press Ctrl-C/SIGKILL and see if it resets correctly. It will need some time from me to report what I find, on macOS/Windows/Linux. I am still unsure how to write a testcase for this, this is a 'noob' issue for me to get acquainted with the code.

I am going to try testing this with some signal handling code as well and report what I found. But this isn't an uncommon issue. A lot of commands fail and I usually run stty sane at that point before typing stuff on the Terminal again.

@rbtcollins
Copy link
Contributor

I don't think at_exit is the right way to tackle this. It's kindof sad Rust is getting at_exit hooks really. But I digress.

A simple signal handler on unix family OSes, and SetConsoleCtrlHandler on Windows, can call the term reset function we want - but ideally we'd be integrated into stack unwinding to allow cleaning up of held locks etc - while we should be crash safe, we can be more efficient sometimes if we have cleanup code :)

@rbtcollins
Copy link
Contributor

@workingjubilee
Copy link
Member

workingjubilee commented Jun 9, 2021

I don't think at_exit is the right way to tackle this. It's kindof sad Rust is getting at_exit hooks really. But I digress.

That's because it is not. This PR relies on the unstable-internal rt feature which is not meant to be used by ordinary consumers of the Rust standard library which are not directly implementing libraries and drivers for the Rust compiler or instrumenting the compiler and its runtime.

I will generally wait for the at_exit to get merged to stable/beta at this point. This isn't a priority for 1.24.x, but the reset handler would have to be called somewhere.

For the above-mentioned reasons, this is extremely unlikely to happen.
See https://doc.rust-lang.org/unstable-book/library-features/rt.html

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.

rustup-init leaves user with colored terminal
4 participants