-
Notifications
You must be signed in to change notification settings - Fork 507
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
Parallel #1738
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apart from style I have one remark, coming from what you said about early exit when one of the tasks fails.
}; | ||
if config.parallel { | ||
scope.spawn(run); | ||
} else if run() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I found it confusing when reading the code to expect true
for the error case, but that might just be me.
if config.parallel { | ||
scope.spawn(run); | ||
} else if run() { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If what you wanted to achieve is an early exit when the first task fails, this won't work in this implementation.
A thread::scope
(also crossbeam::scope
) will wait for all spawned threads to finish before exiting the scope.
I also tested this manually.
if parallel { | ||
scope.spawn(run); | ||
} else if run() { | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above, the early exit won't work like this.
@syphar Ah, good point about early termination. We definitely want that, since presumably, if users are using parallel execution, they have some long-running recipes and want them to terminate early. Hmm, that's annoying. There's no way to terminate a thread from outside of a thread in Rust, so the only way we could do this is by having a channel, and then when spawning commands doing something like: loop {
match rx.recv_timeout(Duration::from_millis(10)) {
Err(RecvTimeoutErr::Timeout) => {}
Err(RecvTimout::Disconnected) => { child.kill(); return Ok(None); }
Ok(()) => { child.kill(); return Ok(None); }
}
match child.try_wait() {
Ok(None) => {},
Ok(Some(status)) => return Ok(Some(status)),
Err(err) => return Err(err),
}
} |
Other issues, which I haven't thought about:
|
Yeah, I imagine that would be a valid approach, killing threads is not really nice, but in case of |
From what I remember when testing dependencies, the parallel threads where handled as a group, so all dependencies of these were handled before handling the first of them. And everything depending on any one of the parallel tasks were waiting for the whole group to finish. I might misremember this, definitely needs testing.
I don't know enough about stdout buffering to know how exactly that behaviour would be, we might need to pipe the outpt and then take care of intermingling the lines properly? Then we could even add prefixes. |
Generally, I'm happy to send time on some of the topics here, which do you want to pick? Should I pick? |
Version of #1562 using rust's native scoped threads.