De-stabilize `thread::scoped` and friends #24385

aturon · 2015-04-13T19:14:01Z

Issue #24292 demonstrates that the scoped API as currently offered can
be memory-unsafe: the JoinGuard can be moved into a context that will
fail to execute destructors prior to the stack frame being popped (for
example, by creating an Rc cycle).

This commit reverts the APIs to unstable status while a long-term
solution is worked out.

(There are several possible ways to address this issue; it's not a
fundamental problem with the scoped idea, but rather an indication
that Rust doesn't currently provide a good way to ensure that
destructors are run within a particular stack frame.)

r? @alexcrichton

aturon · 2015-04-13T19:15:08Z

Per analysis on crates.io, we do not expect this change to impact much code in practice.

Note also that scoped is probably best offered in the long run as part of a framework for lightweight parallel tasks. It implies fork/join parallelism, which wants a pool at least, if not work stealing as well.

Issue rust-lang#24292 demonstrates that the `scoped` API as currently offered can be memory-unsafe: the `JoinGuard` can be moved into a context that will fail to execute destructors prior to the stack frame being popped (for example, by creating an `Rc` cycle). This commit reverts the APIs to `unstable` status while a long-term solution is worked out. (There are several possible ways to address this issue; it's not a fundamental problem with the `scoped` idea, but rather an indication that Rust doesn't currently provide a good way to ensure that destructors are run within a particular stack frame.) [breaking-change]

alexcrichton · 2015-04-13T20:24:09Z

@bors: r+ 6399bb4 p=100

aturon · 2015-04-13T20:34:00Z

@bors: r-, going to generalize spawn bit along the way.

bors · 2015-04-13T20:38:15Z

⌛ Testing commit 6399bb4 with merge e627752...

aturon · 2015-04-13T20:38:33Z

@bors: r- 6399bb4

bors · 2015-04-13T21:09:52Z

💔 Test failed - auto-mac-64-opt

`thread::spawn` was previously restricted to closures that return `()`, which limited the utility of joining on a spawned thread. However, there is no reason for this restriction, and this commit allows arbitrary return types. Since it introduces a type parameter to `JoinHandle`, it's technically a: [breaking-change] However, no code is actually expected to break.

aturon · 2015-04-13T22:51:24Z

@alexcrichton Now with spawn adjustments and test fixes; re-r?

(Still running make check locally but it's pretty far along now.)

seanmonstar · 2015-04-13T23:24:32Z

Orly? This'll break hyper, at least. @reem

alexcrichton · 2015-04-13T23:32:14Z

@bors: r+ a94040f

bors · 2015-04-13T23:52:57Z

⌛ Testing commit a94040f with merge 05c52dd...

bors · 2015-04-13T23:57:55Z

💔 Test failed - auto-win-32-nopt-t

alexcrichton · 2015-04-14T00:02:19Z

@bors: retry

On Mon, Apr 13, 2015 at 4:57 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-32-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-32-nopt-t/builds/4157

—
Reply to this email directly or view it on GitHub
#24385 (comment).

bors · 2015-04-14T00:04:53Z

⌛ Testing commit a94040f with merge a492093...

bors · 2015-04-14T00:07:49Z

💔 Test failed - auto-win-32-opt

alexcrichton · 2015-04-14T00:08:39Z

@bors: retry

On Mon, Apr 13, 2015 at 5:07 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-32-opt
http://buildbot.rust-lang.org/builders/auto-win-32-opt/builds/4147

—
Reply to this email directly or view it on GitHub
#24385 (comment).

bors · 2015-04-14T02:46:06Z

⌛ Testing commit a94040f with merge 9103a0e...

aturon · 2015-04-14T03:27:41Z

@bors: r-

There are some doc issues, need to rewrite the concurrency chapter.

bors · 2015-04-14T03:49:27Z

💔 Test failed - auto-win-64-nopt-t

reem · 2015-04-14T08:15:26Z

@seanmonstar our use in hyper can be adapted to spawn, but it does cost some additional Arcs where we can get away with & today.

aturon · 2015-04-14T15:16:11Z

@bors: r=alexcrichton p=100

bors · 2015-04-14T15:16:12Z

📌 Commit a9fd41e has been approved by alexcrichton

pythonesque · 2015-04-14T16:39:04Z

Given that both of the proposed solutions (unless I'm missing something) change the required bounds of Rc and Arc, shouldn't they be destabilized as well?

aturon · 2015-04-14T16:54:30Z

@pythonesque

Given that both of the proposed solutions (unless I'm missing something) change the required bounds of Rc and Arc, shouldn't they be destabilized as well?

Only one of the proposed solutions involves changing these types, and for that approach to work it would need to be backwards-compatible anyway. Essentially, it would introduce a new marker trait that all existing types would satisfy automatically, and that was assumed automatically in most contexts (a bit like Sized is today). Adding such a bound to Rc/Arc would then not be a breaking change.

However, that's a fairly drastic piece of design, and a much simpler solution is just to not use RAII for scoped. You can instead use an API that works with an additional closure, executing the joins after the closure runs. For example, we used to have a "finally" API that did something like:

fn foo() {
    finally(|| {
        // code to run now
    }, || {
        // code to run afterwards, even on a panic
    }
}

We have some specific ideas about how to make a nice API along these lines work with scoped.

bors · 2015-04-14T16:57:16Z

⌛ Testing commit a9fd41e with merge 203fd1c...

pythonesque · 2015-04-14T17:41:42Z

Perhaps I misinterpreted the comment about "like Arena" to mean that it would entail adding a lifetime to Rc.

The Leak solution is only backwards-compatible if it is made a special, compiler-supported trait, like ?Sized. The straightforward solution is to do it in userspace, but that only works if Rc and Arc are made to require the bound beforehand.

Absent such a solution, the existence of Rc and Arc means that a userspace library cannot even define its own set of safe abstractions that work like the old API, which is more flexible than a naive closure-based one. For example: you can return a JoinGuard from a function, store it on the heap, or choose the order in which the guards are joined. I guess I'd have to see what you had in mind for the new API.

The other reason I think it's worth considering Leak is that I suspect the ability to declare that a destructor really does have to be run (with the usual caveats around abort) is important for other cases than JoinGuard. The fact that Rust doesn't guarantee that is kind of unfortunate, and this would provide a way to recover that somewhat.

Edit: I don't think ?Leak would be backwards-compatible anyway, since users can themselves define smart pointers like Rc. This is a decision that really needs to be made now. Rc and Arc are responsible for a ton of special cases and break a lot of assumptions, so putting a bound on what they can contain makes a lot of sense (particularly because while user libraries can add whatever OIBITs they want to exclude the types altogether, e.g. you could easily add a trait NotRefCounted and opt out of it for Rc and Arc, they can do nothing to control what those types accept in the first place).

(I believe a way to justify building this into the compiler might be to treat !Leak as equivalent to Linear, since clearly a linear type should not be able to hide in Rc or Arc; given this correspondence, one could also say something like unsafe impl<'a, T> Leak for &'a mut T where T: 'a, unsafe impl<'a, T> Leak for &'a T where T: 'a with some degree of confidence, recovering the ability to store, e.g., a &mut JoinGuard or &JoinGuard in a Rc, which I believe is safe).

bors · 2015-04-14T17:55:43Z

💔 Test failed - auto-linux-64-opt

Conflicts: src/libstd/thread/mod.rs src/test/bench/shootout-mandelbrot.rs src/test/bench/shootout-reverse-complement.rs src/test/run-pass/capturing-logging.rs src/test/run-pass/issue-9396.rs src/test/run-pass/tcp-accept-stress.rs src/test/run-pass/tcp-connect-timeouts.rs src/test/run-pass/tempfile.rs

alexcrichton · 2015-04-15T03:17:30Z

Landed in #24433

bors · 2015-04-15T03:20:34Z

☔ The latest upstream changes (presumably #24433) made this pull request unmergeable. Please resolve the merge conflicts.

rust-highfive assigned alexcrichton Apr 13, 2015

aturon force-pushed the unstable-scoped branch from fe993fd to 6399bb4 Compare April 13, 2015 19:15

aturon force-pushed the unstable-scoped branch from ff10152 to a94040f Compare April 13, 2015 22:50

aturon force-pushed the unstable-scoped branch from a94040f to a9fd41e Compare April 14, 2015 15:15

Fallout: move from scoped to spawn

e72adca

aturon force-pushed the unstable-scoped branch from a9fd41e to e72adca Compare April 15, 2015 01:27

alexcrichton closed this Apr 15, 2015

alexcrichton mentioned this pull request Apr 16, 2015

RFC: Alter mem::forget to be safe rust-lang/rfcs#1066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

De-stabilize `thread::scoped` and friends #24385

De-stabilize `thread::scoped` and friends #24385

aturon commented Apr 13, 2015

aturon commented Apr 13, 2015

alexcrichton commented Apr 13, 2015

aturon commented Apr 13, 2015

bors commented Apr 13, 2015

aturon commented Apr 13, 2015

bors commented Apr 13, 2015

aturon commented Apr 13, 2015

seanmonstar commented Apr 13, 2015

alexcrichton commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

alexcrichton commented Apr 14, 2015

bors commented Apr 14, 2015

bors commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

bors commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

reem commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

pythonesque commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

pythonesque commented Apr 14, 2015

bors commented Apr 14, 2015

alexcrichton commented Apr 15, 2015

bors commented Apr 15, 2015

De-stabilize thread::scoped and friends #24385

De-stabilize thread::scoped and friends #24385

Conversation

aturon commented Apr 13, 2015

aturon commented Apr 13, 2015

alexcrichton commented Apr 13, 2015

aturon commented Apr 13, 2015

bors commented Apr 13, 2015

aturon commented Apr 13, 2015

bors commented Apr 13, 2015

aturon commented Apr 13, 2015

seanmonstar commented Apr 13, 2015

alexcrichton commented Apr 13, 2015

bors commented Apr 13, 2015

bors commented Apr 13, 2015

alexcrichton commented Apr 14, 2015

bors commented Apr 14, 2015

bors commented Apr 14, 2015

alexcrichton commented Apr 14, 2015

bors commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

reem commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

pythonesque commented Apr 14, 2015

aturon commented Apr 14, 2015

bors commented Apr 14, 2015

pythonesque commented Apr 14, 2015

bors commented Apr 14, 2015

alexcrichton commented Apr 15, 2015

bors commented Apr 15, 2015

De-stabilize `thread::scoped` and friends #24385

De-stabilize `thread::scoped` and friends #24385