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

Make job queue channel bounded #6294

Closed
wants to merge 1 commit into from

Conversation

ishitatsuyuki
Copy link
Contributor

Reference: #6197

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks for the PR! I would be wary to merge this though as I'm not sure it wouldn't lead to deadlock and/or unpredictable behavior, and in reality I don't think we'll ever run into the problem where child threads outpace the main thread that much to make this that necessary

@ishitatsuyuki
Copy link
Contributor Author

I'd like to note that it definitely can outpace, like when you're using a pipe-based tool that doesn't aggressively drain the output (I remember less did that?), which makes the main thread blocked. Thus backpressure is important here.

Regarding deadlocks, I somewhat understand your concern, but I'm not sure if there's a concrete case. In particular, do we communicate back to the child thread? In that case, we may want to visit that to ensure deadlocks don't happen.

@alexcrichton
Copy link
Member

I could go either way on the "whether this is necessary" part, but to weed out potential deadlock issues I think it'd be best to test this out with a capacity of zero or one and then test this one some large projects (like Servo) with a lot happening in parallel and perhaps with a variety of -v flags as well.

These sorts of deceptively small changes have all classically created a lot of headache down the road as bugs are tracked down in one way or another.

@ishitatsuyuki
Copy link
Contributor Author

So yeah, it actually deadlocked with capacity=0 during tests. I'll try to figure out what dataflow is causing this.

@ishitatsuyuki
Copy link
Contributor Author

Closing this: I don't have much time to refactor the code to remove all cases of deadlock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants