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

Finish runtime removal #18967

Merged
merged 10 commits into from
Nov 21, 2014
Merged

Finish runtime removal #18967

merged 10 commits into from
Nov 21, 2014

Conversation

aturon
Copy link
Member

@aturon aturon commented Nov 15, 2014

This PR completes the removal of the runtime system and green-threaded abstractions as part of implementing RFC 230.

Specifically:

  • It removes the Runtime trait, welding the scheduling infrastructure directly to native threads.
  • It removes libgreen and libnative entirely.
  • It rewrites sync::mutex as a trivial layer on top of native mutexes. Eventually, the two modules will be merged.
  • It hides the vast majority of std::rt.

This completes the basic task of removing the runtime system (I/O and scheduling) and components that depend on it.

After this lands, a follow-up PR will pull the rustrt crate back into std, turn std::task into std::thread (with API changes to go along with it), and completely cut out the remaining startup/teardown sequence. Other changes, including new TLS and synchronization are in the RFC or pre-RFC phase.

Closes #17325
Closes #18687

[breaking-change]

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@alexcrichton
Copy link
Member

Good lord that diff. 💯

@@ -9,8 +9,6 @@ Source layout:
| `libcore/` | The Rust core library |
| `libdebug/` | Debugging utilities |
| `libstd/` | The standard library (imported and linked by default) |
| `libgreen/` | The M:N runtime library |
| `libnative/` | The 1:1 runtime library |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, if I have more comments, feel free to just remove this file entirely. It's long since needed deletion.

@alexcrichton
Copy link
Member

cc @larsbergstrom and @metajack on the libgreen removal

@alexcrichton
Copy link
Member

This looks fantastic to me, nice work @aturon!

cc @brson on the semantic change of exiting a program as soon as one task exits. That's a fairly major semantic change, but I think that it only makes sense. We're no longer assuming that Rust is in control of all threads, so I don't think we can provide a guarantee for programs that we only exit when all threads have exited.

@bill-myers
Copy link
Contributor

Is it memory safe to return from main before other threads have exited?

The problem is that it seems that returning from main will run global C++ destructors before killing the process, which means that other running threads can access objects that are being destroyed.

An alternative could be to not run them at all, either by explicitly exiting the process or exiting the main thread (which will result in waiting for all threads to exit) by calling pthread_exit or ExitThread in the main thread.

Otherwise, one could track and wait for all Rust-spawned threads, and assume other libraries have it correct, or wait for all threads using platform-specific APIs (e.g. /proc/self/task), which might however hang the process if any C/C++ library does not stop its threads.

@alexcrichton
Copy link
Member

Oh I was unaware of the semantics of pthread_exit and ExitThread! It looks like dealing with the process exit code status may be... interesting, but we may perhaps be able for it to work. I'm not super worried about memory safety and C++ destructors because they're... C++ destructors! We don't support global destructors/constructors in Rust yet, which is what I would be more worried about if so.

@bill-myers
Copy link
Contributor

Oops, indeed it doesn't work with non-zero error codes...

Regarding destructors, they are indeed C++ destructors, but it seems quite reasonable to have, for instance, a safe Rust binding to a C++ library that does lookups in a global std::unordered_map variable that is populated once, and it seems that would be unsafe if that global variable is destroyed while a thread is running.

Probably rare and likely not exploitable, but it seems something to think about.

Overall, it seems best to just keep current semantics by maintaining a list of unjoined Rust-created threads and joining any that is still running after main returns.

The main executable might not be a Rust program, so this probably means registering a global C++ destructor or using atexit to do that.

Immediate thread cancellation would be even better since it avoids the risk of freezing the main program on exit due to a Rust library being used, but I don't think Rust can do that now or in the foreseeable future (it requires async task failure).

@aturon
Copy link
Member Author

aturon commented Nov 15, 2014

@bill-myers Partly this is a desire to impose little or no special semantics over what the system provides (part of the stance of the Runtime Removal RFC). There's an issue for this concern in particular, although @thestinger doesn't give detailed arguments there.

It seems like it might be worth having a broader debate about this question, since the original RFC didn't specify a semantics here. I would certainly be willing to file an amendment to the RFC to kick off the discussion; @thestinger, it would be helpful if you could lay out in more detail the tradeoffs as you see them.

@thestinger
Copy link
Contributor

Overall, it seems best to just keep current semantics by maintaining a list of unjoined Rust-created threads and joining any that is still running after main returns.

Leaking detached threads until the end of main sounds insane. It's not much better to leak the entire tree of tasks as they block on all of their children. Either way, there's a serious problem with leaking tasks which is similar to the problem of leaking goroutines in Go. This isn't an issue with the sane native threading model where threads can either be detached or explicitly / implicitly (destructor) joined.

@thestinger
Copy link
Contributor

C++ libraries are responsible for being memory safe within the thread model exposed by C++11. There are defect reports filed about how the implementation of global C++ destruction is completely broken, and is the C++ committee's responsibility to cope with that, not Rust's.

@aturon
Copy link
Member Author

aturon commented Nov 15, 2014

@thestinger

Overall, it seems best to just keep current semantics by maintaining a list of unjoined Rust-created threads and joining any that is still running after main returns.

Leaking detached threads until the end of main sounds insane. It's not much better to leak the entire tree of tasks as they block on all of their children.

A couple of things. First, I think @bill-myers's suggestion to keep the current semantics in practice just means holding onto a global atomic counter, as the bookkeeping module was providing -- not literally keeping the threads around.

Second, it'd be helpful to avoid language like "insane" when characterizing other community members's ideas; please keep the arguments technical.

Finally, if you have the chance, I'd like to get more detail on your thoughs about a C-style semantics (as my PR is moving toward) versus the old bookkeeping-based semantics. I agree with your stance on the issue -- my personal instinct is to follow the C tradition and keep things maximally lightweight -- but I'm wondering if you had other technical reasons in mind as well. That would help me write an RFC amendment on the topic.

@bill-myers
Copy link
Contributor

Actually this seems a really hard problem and joining Rust threads doesn't really work.

Because the thing is that the Rust safe code calling the C++ API with a global object might in fact be running in a thread created from C.

But you can't wait for threads created in C, because they may never exit expecting to be killed automatically on process exit.

Also, you can't really forcibly kill all threads because a C++ global destructor might attempt to communicate with a thread.

You can't forcibly kill the whole process either because the main executable could be a C++ executable that expects to have its global destructors run.

The only solution I see is to make EVERY call from Rust to C check a flag and fail if it is called after main returns. This is not ideal from a performance standpoint though.

Oh and this requires to register from a dynamic library something that runs before all C++ global destructors in the process, and probably before all atexit handlers as well, which I have no idea how feasible it is.

Or give up and say that C++ libraries with global objects cannot be safely bound to from Rust.

@bill-myers
Copy link
Contributor

In an nutshell, the problem is this:

  1. C++ library defines a C++ global object that is not modified after initialization and thus thread-safe and exports a C API that queries it, which Rust binds to
  2. The main C executable creates a thread that calls a Rust function that has a loop that in every iteration uses the binding to call the C API in the library above which accesses the C++ global object
  3. The main C executable's main() returns, which causes the C++ global object to be destroyed
  4. The C-created thread is still running the loop in Rust code with calls the safe Rust-bound C API and accesses the C++ global object and crashes

I guess this is an issue in all versions of Rust, btw.

@thestinger
Copy link
Contributor

Finally, if you have the chance, I'd like to get more detail on your thoughs about a C-style semantics (as my PR is moving toward) versus the old bookkeeping-based semantics. I agree with your stance on the issue -- my personal instinct is to follow the C tradition and keep things maximally lightweight -- but I'm wondering if you had other technical reasons in mind as well. That would help me write an RFC amendment on the topic.

It's lighter (lower resource usage, less synchronization), inherently compatible with the POSIX / win32 / C11 / C++11 threading models and is more expressive as you aren't forced to wait for tasks to finish. Since Rust doesn't have support for non-blocking / async IO and all of the things that come with that (selection across channels, timers, signals and IO ops) it really needs tasks to get out of the way. I had originally tried to write playpen in Rust and it turned out to be impossible without making my own standard library. Instead, it's written in C where there's access to the capabilities provided by the platform.

@thestinger
Copy link
Contributor

Second, it'd be helpful to avoid language like "insane" when characterizing other community member's ideas; please keep the arguments technical.

It's helping to avoid mischaracterizing other people's statements and shutting down open and honest discussion with non-technical arguments as you're doing here. I prefer taking a direct approach rather than resorting to passive aggressiveness and deceit, and I don't plan on changing that. I don't see anything in the code of conduct mandating that people mask their opinions about technical issues.

@thestinger
Copy link
Contributor

@bill-myers: C++11 standardized detached threads and code is expected to use them. C++ libraries are responsible for being safe in a world where detached threads are used. These issues were considered during the C++11 development process. A model without detached threads was considered, but ultimately they chose to handle the problem in other ways and made detached threads the default despite that forcing a wrapper around std::thread to use use joined threads correctly.

I don't really see why any of this would be Rust's responsibility, considering that C++ has full support for detached threads and even prefers them. Proposals just like what you are suggesting were explicitly rejected in their standardization process.

@aturon aturon force-pushed the remove-runtime branch 2 times, most recently from bcb46cc to 59a4075 Compare November 16, 2014 05:19
@aturon
Copy link
Member Author

aturon commented Nov 16, 2014

@alexcrichton I've updated this PR to fully remove libnative, removing the bootstrapping gunk, and fixed the test fallout.

I'm comfortable landing this with the change to termination semantics as part of the meaning of the runtime removal RFC. However, I will probably file an amendment or separate RFC to clarify this point and get additional feedback.

@bill-myers
Copy link
Contributor

@thestinger The reason it might be Rust's responsibility is that safe Rust code is not supposed to ever crash or trigger undefined behavior (or do so by calling safe C++ bindings), while C++ can and does just say "don't do this and if you do it's undefined behavior".

Of course saying "bindings to such C++ code must be marked as unsafe" is an acceptable way of handling the issue for Rust.

@aturon aturon force-pushed the remove-runtime branch 2 times, most recently from 4c0dd94 to fef96ab Compare November 20, 2014 06:58
@aturon aturon force-pushed the remove-runtime branch 2 times, most recently from 903ef0c to 40716bf Compare November 20, 2014 22:55
@aturon
Copy link
Member Author

aturon commented Nov 20, 2014

I've updated the PR to temporarily reinstate the bookkeeping system, i.e. to still block shutting down on all Rust threads terminating. This is because, in order to fully remove this semantics, we have to substantially revise the startup/shutdown infrastructure in librustrt. I want to leave that for the next PR.

@alexcrichton re-r? The change was in the first and fifth commits.

This commit removes most of the remaining runtime infrastructure related
to the green/native split. In particular, it removes the `Runtime` trait
and instead inlines the native implementation.

Closes rust-lang#17325

[breaking-change]
With runtime removal complete, there's nothing left of libnative. This
commit removes it.

Fixes rust-lang#18687

[breaking-change]
With runtime removal complete, there is no longer any reason to provide
libgreen.

[breaking-change]
Previously, sync::mutex had to split between green and native runtime
systems and thus could not simply use the native mutex facility.

This commit rewrites sync::mutex to link directly to native mutexes; in
the future, the two will probably be coalesced into a single
module (once librustrt is pulled into libstd wholesale).
Previously, the entire runtime API surface was publicly exposed, but
that is neither necessary nor desirable. This commit hides most of the
module, using librustrt directly as needed. The arrangement will need to
be revisited when rustrt is pulled into std.

[breaking-change]
bors added a commit that referenced this pull request Nov 21, 2014
This PR completes the removal of the runtime system and green-threaded abstractions as part of implementing [RFC 230](rust-lang/rfcs#230).

Specifically:

* It removes the `Runtime` trait, welding the scheduling infrastructure directly to native threads.

* It removes `libgreen` and `libnative` entirely.

* It rewrites `sync::mutex` as a trivial layer on top of native mutexes. Eventually, the two modules will be merged.

* It hides the vast majority of `std::rt`.

This completes the basic task of removing the runtime system (I/O and scheduling) and components that depend on it. 

After this lands, a follow-up PR will pull the `rustrt` crate back into `std`, turn `std::task` into `std::thread` (with API changes to go along with it), and completely cut out the remaining startup/teardown sequence. Other changes, including new [TLS](rust-lang/rfcs#461) and synchronization are in the RFC or pre-RFC phase.

Closes #17325
Closes #18687

[breaking-change]

r? @alexcrichton
@bors bors closed this Nov 21, 2014
@bors bors merged commit 32c3d02 into rust-lang:master Nov 21, 2014
Centril added a commit to Centril/rust that referenced this pull request Sep 15, 2019
…entril

Warn on no_start, crate_id attribute use

These attributes are now deprecated; they don't have any use anymore.

`no_start` stopped being applicable in 3ee916e as part of rust-lang#18967. Ideally we would've removed it pre-1.0, but since that didn't happen let's at least mark it deprecated.

`crate_id` was renamed to `crate_name` in 50ee1ec as part of rust-lang#15319. Ideally we would've followed that up with a removal of crate_id itself as well, but that didn't happen; this PR finally marks it as deprecated at least.

Fixes rust-lang#43142 and resolves rust-lang#43144.
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.

native::run is useless Remove libgreen and runtime abstractions
9 participants