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

std: Run at_exit cleanup on process::exit #28069

Merged
merged 1 commit into from Sep 4, 2015

Conversation

alexcrichton
Copy link
Member

This adds a call to rt::cleanup on process::exit to make sure we clean up
after ourselves on the way out from Rust.

Closes #28065

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

I remember this being super hard to land in the past, but I forget precisely why, and I can always try to patch up any bot failures!

@brson
Copy link
Contributor

brson commented Aug 28, 2015

I'm concerned that this may tie us to C semantics in a deeper way than anything else presently in std. Are Rust exit handlers going to be defined always as compatible with the C runtime's atexit handlers? What if there are situations where std can't depend on C atexit?

@alexcrichton
Copy link
Member Author

Hm, could you elaborate on your concerns a bit more? std::rt::at_exit is still unstable and there's no way you can register your own custom exit handlers, and otherwise I see this as basically "we will try to run our own cleanups as aggressively as possible".

I don't think we necessarily need to provide any guarantees just yet other than that we'll try our best to run them, not necessarily the exact same semantics as libc. If there are situations where we can't depend on C atexit (like on Windows which I need to fix here) then we'll just call cleanup manually on process::exit

@retep998
Copy link
Member

At the very least on Windows, don't rely on the libc atexit (since we don't use the libc exit). Instead just have process::exit call the appropriate cleanup.

@alexcrichton alexcrichton force-pushed the rt-atexit branch 4 times, most recently from ebf06b3 to 5e066ae Compare August 29, 2015 00:18
@alexcrichton
Copy link
Member Author

I've updated to call cleanup manually on Windows and only use atexit on Unix.

@ranma42
Copy link
Contributor

ranma42 commented Aug 29, 2015

The commit message seems to indicate that a call to rt::at_exit::cleanup has been removed, but from the commit diff it is not clear that this happened.

Wouldn't it be possible to avoid splitting the implementation for win/unix and just use the windows implementation for both? This would also reduce the dependence on the semantics of the C runtime, so it might also address some of the concerns expressed by @brson.

diff --git a/src/libstd/process.rs b/src/libstd/process.rs
index be921d9..3d5090f 100644
--- a/src/libstd/process.rs
+++ b/src/libstd/process.rs
@@ -21,6 +21,7 @@ use fmt;
 use io::{self, Error, ErrorKind};
 use path;
 use sync::mpsc::{channel, Receiver};
+use sync::Once;
 use sys::pipe::{self, AnonPipe};
 use sys::process as imp;
 use sys_common::{AsInner, AsInnerMut, FromInner, IntoInner};
@@ -582,6 +583,8 @@ impl Child {
 /// to run.
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn exit(code: i32) -> ! {
+    static CLEANUP: Once = Once::new();
+    CLEANUP.call_once(|| unsafe { ::rt::cleanup() });
     ::sys::os::exit(code)
 }

@nagisa
Copy link
Member

nagisa commented Aug 30, 2015

We could piggy-back on the TLS destructors to have unified implementation on both platforms. If that is unacceptable, it seems that the same scheme as used to register TLS destructors could be used to schedule cleanup on windows as well.

It also sounds much more reliable. I’m not sure about UNIX, but at least on windows cleanup would logically run whatever way you’d exit the process.

@retep998
Copy link
Member

Note that relying on the TLS destructor mechanism means that #28111 would have to be fixed.

@alexcrichton
Copy link
Member Author

@ranma42 I would prefer to take the more idiomatic road on Unix to ensure that if Rust is statically linked into another application that we play as nicely as possible there (e.g. our dtors will run via atexit when the whole app exits). We can in theory provide a callback in the future to manually run Windows dtors, but for now it seems ok to not worry about that.

@nagisa unfortunately running TLS dtors when main exits is quite difficult. For example pthread TLS keys don't seem to run dtors on process exit (#28129)

@brson
Copy link
Contributor

brson commented Aug 31, 2015

@alexcrichton The semantic change here is: 'This ensures that cleanup handlers are
run on process exit via the exit function.' So, after this we are promising that, for all Rust code that uses the standard library, if you call the 'C' exit function, certain things happen in Rust. This is coupling the C runtime to the Rust runtime in a concrete and subtle way, and I'm concerned about whether this contract can be fulfilled reliably on all platforms where C and Rust are expected to interoperate (and also whether this level of coupling is something we are willing to spec).

@nagisa
Copy link
Member

nagisa commented Aug 31, 2015

We could totally implement process::exit via direct syscall, pretty much like on Windows.

@alexcrichton
Copy link
Member Author

@brson I'm not sure I quite understand these concerns. The alternative is that we intentionally avoid calling standard C functions, but that seems... bad?

@brson
Copy link
Contributor

brson commented Sep 1, 2015

@alexcrichton The difference between at_exit and arbitrary standard C functions is that at_exit has C-runtime-specific side-effects.

@alexcrichton alexcrichton changed the title std: Schedule at_exit cleanup via libc::atexit std: Run at_exit cleanup on process::exit Sep 3, 2015
@alexcrichton alexcrichton force-pushed the rt-atexit branch 2 times, most recently from 84c9e3f to 10dcf5c Compare September 3, 2015 00:19
@alexcrichton
Copy link
Member Author

OK, after some discussion with @brson offline I've not moved this to avoid atexit entirely to prevent differences across platforms, and now this just adds a standard call to rt::cleanup from process::exit.

I did a bit of an audit to make sure it's ok to continue using all the pieces that are cleaned up even after they've been cleaned up (as this can all happen concurrently) and we should be good to go there.

This adds a call to `rt::cleanup` on `process::exit` to make sure we clean up
after ourselves on the way out from Rust.

Closes rust-lang#28065
@brson
Copy link
Contributor

brson commented Sep 3, 2015

@bors r+ thanks

@bors
Copy link
Contributor

bors commented Sep 3, 2015

📌 Commit 04c09f9 has been approved by brson

bors added a commit that referenced this pull request Sep 4, 2015
This adds a call to `rt::cleanup` on `process::exit` to make sure we clean up
after ourselves on the way out from Rust.

Closes #28065
@bors
Copy link
Contributor

bors commented Sep 4, 2015

⌛ Testing commit 04c09f9 with merge 6f1014f...

@bors bors merged commit 04c09f9 into rust-lang:master Sep 4, 2015
@alexcrichton alexcrichton deleted the rt-atexit branch October 21, 2015 06:15
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

9 participants