Change thread_worker library to propagate panics by default

We have a small thread_worker for spawning background threads. It's main purpose is to force clean thread shutdown: if you don't join the thread, it might be forcefully terminated when the process exists, skipping destructors. Leaving threads unjoined is also bad for reliable unit-testing, and in general bad for overall architecture of the system.

A slight problem with this library is that it turns a thread's panic into Result by forcing you to use the shutdown method. I now think this is a bad idea: it's usually best to escalate the panic instead of catching it, even on the thread boundary. We need to switch "panic, if child thread panicked" behavior.

To do this, we need the following changes:

introduce a wrapper around thread::spawn and thread::JoinHandler which joins the thread automatically in Drop, propagating the panic from the child thread if the parent thread is not panicking itself. We should use this wrapper in cases where we spawn bare threads, like here
Remove WorkerHandle struct (which exists solely to join the thread) and instead add our JoinHandler (what's a good name here? ScopedThread, ThreadGuard?) to the Worker
Change Worker's shutdown method to return a pair of (ScopedThread, Receiver)
update usages of thead_worker to the new API, paying close attention to possible deadlocks. Specifically, channel senders should be dropped before ScopedThread, even in case of a panic.

@stjepang

cc @stjepang, this is very related to rust-lang/rust#48830 and shows an interesting interaction between channels and threads (drop sender prior to joining).

Slightly tangential, but, if you are interested in threads and clean cancellation, there's an interesting discussion over here: https://trio.discourse.group/t/structured-concurrency-in-rust/73

drop sender prior to joining

Hm, this seems fun indeed, looks like the pattern we want is

let (sender, receiver) = channel();
let thread = spawn(move || {
    for event in receiver {
        ...
    }
})

// subtle, force sender to be dropped *before* thread, 
// avoiding deadlock if *this* thread panics.
let sender = sender;


// Alternative way to spell this

let thread;
let (sender, receiver) = channel();
thread = spawn(move || {
    for event in receiver {
        ...
    }
})

That's a super uncomfortable level of subtlety, can we do better?

We could just use std::mem::drop manually.

That works for the happy case, but not for the panics, a panic will cause a deadlock

automatically wait for worker threads

Merge #833

Navigation Menu

Repository navigation

Change `thread_worker` library to propagate panics by default #817

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Change thread_worker library to propagate panics by default #817

Description

Activity

matklad commented on Feb 13, 2019

matklad commented on Feb 13, 2019

matklad commented on Feb 13, 2019

DJMcNab commented on Feb 13, 2019

matklad commented on Feb 13, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Change `thread_worker` library to propagate panics by default #817