-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Apply advisory locks when building source distributions #3525
Conversation
@@ -396,6 +396,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { | |||
hashes: HashPolicy<'_>, | |||
client: &ManagedClient<'_>, | |||
) -> Result<BuiltWheelMetadata, Error> { | |||
let _lock = cache_shard.lock().map_err(Error::CacheWrite)?; |
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.
Realizing... I think I need this to be async? Is that right @ibraheemdev?
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.
Looks like you hold it across await
points, so yeah.
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.
It’s not a mutex though, it’s a flock. I’d need to do some research.
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.
Hm.. aren't you still yielding control? So some other task could hit this lock and block without awaiting and returning control to the event loop and consequently to this task? e.g.
- Task A takes lock
- Task A yields to event loop
- Task B attempts to take lock
- Lock is not available so Task B is blocked but has not yielded to the event loop
- Task A is never returned to
- Deadlock
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.
Wrapped it in a tokio task.
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.
Yeah I agree with this.
d4444f6
to
f20d3b5
Compare
eda9cb3
to
2227e2b
Compare
f20d3b5
to
1f1e274
Compare
2227e2b
to
348aa34
Compare
c940843
to
b61b014
Compare
710119e
to
a14de20
Compare
a14de20
to
a7bb014
Compare
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.
LGTM.
Summary
I don't love this, but it turns out that setuptools is not robust to parallel builds: pypa/setuptools#3119. As a result, if you run uv from multiple processes, and they each attempt to build the same source distribution, you can hit failures.
This PR applies an advisory lock to the source distribution directory. We apply it unconditionally, even if we ultimately find something in the cache and don't do a build, which helps ensure that we only build the distribution once (and wait for that build to complete) rather than kicking off builds from each thread.
Closes #3512.
Test Plan
Ran: