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

Apply advisory locks when building source distributions #3525

Merged
merged 2 commits into from May 13, 2024

Conversation

charliermarsh
Copy link
Member

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:

#!/bin/bash
make_venv(){
    target/debug/uv venv $1
    source $1/bin/activate
    target/debug/uv pip install opentracing --no-deps --verbose
}

for i in {1..8}
do
   make_venv ./$1/$i &
done

@charliermarsh charliermarsh added the bug Something isn't working label May 11, 2024
@@ -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)?;
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

@charliermarsh charliermarsh force-pushed the charlie/source-tree branch 2 times, most recently from d4444f6 to f20d3b5 Compare May 11, 2024 17:31
@charliermarsh charliermarsh force-pushed the charlie/source-tree branch 5 times, most recently from c940843 to b61b014 Compare May 11, 2024 21:05
@charliermarsh charliermarsh force-pushed the charlie/build-lock branch 4 times, most recently from 710119e to a14de20 Compare May 13, 2024 00:26
crates/uv-distribution/src/source/mod.rs Outdated Show resolved Hide resolved
Base automatically changed from charlie/source-tree to main May 13, 2024 14:03
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@charliermarsh charliermarsh merged commit 9a92a3a into main May 13, 2024
44 checks passed
@charliermarsh charliermarsh deleted the charlie/build-lock branch May 13, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wheel Build failures when installing via multiple simultaneous UV instances
4 participants