Incompatibility of Rust's stdlib with Coroutines

[lhecker]

The issue

thread_local! is used in the stdlib, which does not work well with Coroutines. Repeated access to a TLS variable inside a method might be cached and optimized by LLVM. If a coroutine is transferred from one thread to another this can lead to problems, due to a coroutine still using the cached reference to the TLS storage from the previous thread.

What is not the issue

TLS being incompatible with coroutines for the most part (e.g. here) is well known and not an issue per se. You want to use rand::thread_rng() with coroutines? Just use rand::StdRng::new() instead! Most of the time it's just quite easy to circumvent the TLS by simply using something different. This is not true for the stdlib though. One way or the other you're using it somewhere probably.

Possible solutions

• Add a option akin to RFC 1513 to replace the stdlib with one with a builtin "libgreen". This might actually be much more practicable than it sounds at first, since the overhead of implementating this is not much larger than of the other options.
• Add a option akin to RFC 1513 to control inlining of TLS access at compile time.
• Make it possible to hook into thread_local!. I think that this could be hard to achieve in a performant way though.
• Reduce the usage of TLS inside the stdlib and instead let crates use it as they please. Panic and unwind semantics could for instance be changed to match C++. This would obviate PANIC_COUNT and it's wonky implementation and still make entirely sure that a stack is unwound twice. Other uses of TLS inside the stdlib could be wrapped inside inline(never) without causing large overheads.
• Possibly some other way? I read that TLS variables are rendered as globals inside LLVM. If we mark the suspension function as "can write to any memory location", we could make LLVM stop caching the TLS access...

I hope we can find a solution for this as this is really a huge problem for using stackful coroutines with Rust and who doesn't want "Go" but with Rust's syntax, eh? 😉

[nagisa]

You could add a yet another way to panic as per rust-lang/rfcs#1513, in a way which circumvents any catch_unwind.

[arielb1]

@lhecker

If catch_unwind did not run user code within the with, would it be better? In that case you still could not switch coroutines during a panic (which you can't do currently anyway, because of PANIC_COUNT), but you would not have the reference problem.

[lhecker]

@nagisa Would that work across crates? I'm also not sure if that would be easier than simply splitting up catch_unwind into it's 2 atomic parts.

@arielb1 Yeah it would solve the reference problem, but then it would set the prev to the wrong thread. I think this could be the easiest solution though as long as you make sure that thread::panicking() is false before you call catch_unwind(). If no other alternative gets accepted I'll submit a PR for that.

[nagisa]

Would that work across crates?

The RFC requires only one panicking mechanism to be used in the final binary.

I'm also not sure if that would be easier than simply splitting up catch_unwind into it's 2 atomic parts.

If we want to think about that at all we gotta act fast, because 1.9 with a stable catch_unwind is being released in 2016-05-26.

Make it possible to hook into thread_local!, thus solving the problem entirely. I think that this could be hard to achieve in a performant way though.

Something towards this would be my preferred solution, but keep in mind that thread_local! itself is a wrapper over many layers of abstractions over TLS (any of which could be used anywhere and by anyone).

[lhecker]

I don't think another panic behaviour is the right choice here @nagisa, since changing the "obversable" behaviour of panic isn't really what coroutine implementations need. Rather the API or the implementation has to change. Maybe I misunderstood your idea though...

Hooking into thread_local! would also be my preferred choice if it can be done in a way that doesn't hinder performance. To get a good performance it could be done like rust-lang/rfcs#1513 though, right? (I.e. replacing the TLS lookup function at compile time.)

Apart from that I think that @arielb1's idea is quite good and could be implemented right now without causing any regressions.

[zonyitoo]

I still cannot get the idea behind the PANIC_COUNT, because it just allows people to panic inside Drop::drop while panicking. But why is that?

If we can remove the PANIC_COUNT, and just store a boolean flag in TLS to indicate that whether this thread is panicking, this issue could be solved. In the catch_unwind, you can restore the flag so that you can panic again.

I don't know why we need to accept panic in drop?

[sfackler]

PANIC_COUNT doesn't "allow" people to panic inside drop while panicking - it allows the runtime to determine that has happened so it can abort instead of re-unwinding.

PANIC_COUNT is a number stored in TLS, and I'm not sure how changing that to a bool would materially affect the issues here.

[lhecker]

@sfackler Yeah making it a bool wouldn't solve it probably. But why is this implemented in the "runtime" instead of doing it like C++ with it's noexcept destructors? Is there any technical reason for this? And what about the other solutions I listed in the initial comment?

EDIT: Thanks GitHub for putting the close button right beside the comment button, without adding any confirmation dialogue. UI Design? Anyone? 😐

[zonyitoo]

@sfackler Well, yes. Changing it to bool won't solve all the problems. But, at least, catch_panic will work:

thread_local! { pub static IS_PANICKING: Cell<bool> = Cell::new(false); }

// Here is a function that will be called when panic! happens
// Just like the one in https://github.com/rust-lang/rust/blob/master/src/libstd/panicking.rs#L198
fn on_panic(obj: &(Any+Send), file: &'static str, line: u32) {
    let is_panicking = IS_PANICKING.with(|s| {
        let orig = s.get();
        s.set(true);
        orig
    });

    if is_panicking {
        // Abort right here
        util::dumb_print(format_args!("thread panicked while processing \
                                       panic. aborting.\n"));
        unsafe { intrinsics::abort() }
    }

    // ...
}

// https://github.com/rust-lang/rust/blob/master/src/libstd/sys/common/unwind/mod.rs#L159
pub fn panicking() -> bool {
    IS_PANICKING.with(|s| s.get())
}

// The catch_panic
// https://github.com/rust-lang/rust/blob/master/src/libstd/sys/common/unwind/mod.rs#L131
unsafe fn inner_try(f: fn(*mut u8), data: *mut u8)
                    -> Result<(), Box<Any + Send>> {
    if panicking() {
        // It should not be allowed (to catch panic while panicking)
        unsafe { intrinsics::abort() }
    }

    let mut payload = imp::payload();
    let r = intrinsics::try(f, data, &mut payload as *mut _ as *mut _);

    // Clear the flag because we already caught the panic
    IS_PANICKING.with(|s| s.set(false));

    if r == 0 {
        Ok(())
    } else {
        Err(imp::cleanup(payload))
    }
}

it allows the runtime to determine that has happened so it can abort instead of re-unwinding.

I think this solution can also fulfill this purpose and catch_panic can be used in coroutines now.

[nagisa]

But why is this implemented in the "runtime" instead of doing it like C++ with it's noexcept destructors?

Because unwind in Rust is always a cold path and we do not want to generate extra drop glue just to accomodate for unwinds from drop glue.

I don't know why we need to accept panic in drop?

Because it has perfectly sensible use-cases.

[lhecker]

Because unwind in Rust is always a cold path and we do not want to generate extra drop glue just to accomodate for unwinds from drop glue.

Hmm... Your statement sounds reasonable, but is that based on actual benchmarking?

Because it has perfectly sensible use-cases.

I kind of don't understand that though, because if your destructor can panic, it will one time correctly unwind the thread and one time it will crash the whole program, because the destructor was called as part of a ongoing unwind. I can't say I like that "wonkyness". If you have time: Would you care to point out a valid use case? It's purely optional and out of interest though.

[jonas-schievink]

@lhecker The compiler's own DiagnosticBuilder can panic in its destructor:

rust/src/libsyntax/errors/mod.rs

Lines 354 to 364 in 3157691

	impl<'a> Drop for DiagnosticBuilder<'a> {
	fn drop(&mut self) {
	if !panicking() && !self.cancelled() {
	self.emitter.borrow_mut().emit(&MultiSpan::new(),
	"Error constructed but not emitted",
	None,
	Bug);
	panic!();
	}
	}
	}

[nagisa]

Hmm... Your statement sounds reasonable, but is that based on actual benchmarking?

Implementing nounwind drop glue would simply double the size of binary code used by drop glues. There’s nothing to benchmark here.

If you have time: Would you care to point out a valid use case?

One case, as pointed out by @jonas-schievink, would be ensuring that all objects go out of scope in a valid state. You cannot drop a DiagnosticBuilder without either emitting or cancelling it, because its a compiler bug otherwise. Similar could apply to something like GuaranteedFlushBufWriter which only allows dropping in case it is properly flushed. There’s many more things one could think of.

You still do not want these to just abort because somewhere down the drop chain there might be a drop which would write some restore state/debug logs/user configuration/etc (but it is fine if the application aborts in case those drops panic, because saving such state when panicking is a last ditch effort anyway).

Basically, IME, disallowing panicking in Drop::drop would be a weird and unpleasant restriction. Whereas, an application panicking twice and then aborting as a result of double-panic won’t make the application any more broken, because it is already broken.