Skip to content

Change thread_worker library to propagate panics by default #817

Closed
@matklad

Description

@matklad

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.

Metadata

Metadata

Assignees

Labels

E-mediumfunA technically challenging issue with high impact

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions