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

scoped threads vs thread leaks #2629

Closed
RalfJung opened this issue Oct 28, 2022 · 9 comments · Fixed by #2699
Closed

scoped threads vs thread leaks #2629

RalfJung opened this issue Oct 28, 2022 · 9 comments · Fixed by #2699

Comments

@RalfJung
Copy link
Member

The standard library scoped threads do not use the OS facilities for joining threads, but instead they implement join logic with their own synchronization primitives. (This seems to help with cleaning up thread resources ASAP rather than keeping them around until the scope finishes and all threads are joined.)

Unfortunately this can show up in Miri as a thread leak, if a thread gets scheduled away just after the custom join logic is done, and then the main thread finishes.

For now, a possible work-around is to use -Zmiri-ignore-leaks. But of course it'd be nice to find a better solution. Maybe, after the main thread is done, instead of halting execution immediately we could yield that thread N times (for some suitable N), thus giving other threads a chance to finish?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2022

As an immediate step we should probably add a test for it ^^

The "leaked" threads don't have anything interesting to finish doing, so maybe we could have the scoped cleanup logic mark them as leakable or just join them under cfg(miri)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 28, 2022

Yeah I considered just adding this to the standard library somewhere after main returns:

if cfg!(miri) {
  // Give other threads a chance to finish their work.
  for _ in 0..100 { thread::yield_now(); }
}

But honestly that seemed a bit too hacky.^^ Seems better to do this on the interpreter side.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2022

No I meant... actually join the threads in scoped thread code instead of just cleaning up their resources and letting them run to completion.

@RalfJung
Copy link
Member Author

That would be a much bigger and more annoying set of cfg(miri).

@saethlin
Copy link
Member

Do we have a reproducer for this? It sounds to me like a really tight window?

@RalfJung
Copy link
Member Author

Yes it's a tight window. I definitely saw this happen, which is how I even noticed, but don't remember the exact code. But a test will always risk false negatives here.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 28, 2022

For example, this code

use std::thread;

fn main() {
  thread::scope(|s| {
    s.spawn(|| {});
  });
}

reproduces the problem with seed 9.

But you usually need to try a bunch of seeds to see this happen.

@saethlin
Copy link
Member

saethlin commented Nov 2, 2022

Is there any particular reason to believe that "yield to all other threads on thread exit" isn't something we should/can do in the interpreter instead?

@RalfJung
Copy link
Member Author

RalfJung commented Nov 2, 2022

That's exactly what I suggested in the OP. :)

@bors bors closed this as completed in 837c679 Dec 1, 2022
RalfJung pushed a commit to RalfJung/rust that referenced this issue Dec 3, 2022
refactor scheduler

Refactors the scheduler to use something akin to a generator -- a callback that will be invoked when the stack of a thread is empty, which has the chance to push a new stack frame or do other things and then indicates whether this thread is done, or should be scheduled again. (Unfortunately I think we [cannot use actual generators](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Generators.20that.20borrow.20on.20each.20resume.3F) here.) The interpreter loop is now a proper infinite loop, the only way to leave it is for some kind of interrupt to be triggered (represented as `InterpError`) -- unifying how we handle 'exit when calling `process::exit`' and 'exit when main thread quits'.

The last commit implements an alternative approach to rust-lang/miri#2660 using this new structure. Fixes rust-lang/miri#2629.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants