fix: fix race condition on namespace package installation

[StefanBRas]

Pull Request Check List

Resolves: #9158

• Added tests for changed code.
• Updated documentation for changed code.

There doesn't seem to be any tests that trigger the behavior. I tried raising an error instead of logging a warning and no test failed. Since it's a race condition and requires specific wheels to test, setting up a test wasn't trivial and I just left it in the current untested state. I tested it manually with the reproduction pyproject.toml in the linked issue and it worked fine though.

I think ideally we would raise a FileAlreadyExistsError and handle that accordingly, but since WheelDestination is being used by installer I couldn't see any nice way to do this.

I'm not sure if it was better to copy in the hashing code from installer.utils.copyfileobj_with_hashing as I did or give it an temporary BinaryIO as destination that we would just discard.

[Secrus]

Test is needed.

[StefanBRas]

Test is needed.

Can you give some pointers on how to achieve this then? As I see it I will have to manually create two wheels and then somehow force the race condition to happen. I'm not sure what a feasible way to do this would be, and if it's even safe to allow me to include wheels in the test suite.

[radoering]

Testing the race condition might not be feasible but maybe testing the changed behavior if the path already exists?

[StefanBRas]

Testing the race condition might not be feasible but maybe testing the changed behavior if the path already exists?

I can make a test that verifies that the path is hit, doesn't throw an error and that the output file is as expected, would that be enough? Right now the path is not being hit in any test, so it's still an improvement.

However it requires me to include wheels. Is that okay? Or do I need to make a fixture that builds the wheels from some source data that you can approve?

[radoering]

I can make a test that verifies that the path is hit, doesn't throw an error and that the output file is as expected, would that be enough?

+ checking the RECORD files in the .dist-info folders of the conflicting packages in the venv (that's where the RecordEntry goes).

However it requires me to include wheels. Is that okay? Or do I need to make a fixture that builds the wheels from some source data that you can approve?

I suppose small wheels built specifically for the test are ok. We already have some wheels in tests/fixtures/distributions but I suppose you will not find two matching wheels that fulfill the requirements for the tests.

[StefanBRas]

I've just found a TOCTOU - two threads can hit the target_path.exists() at the same time, get false and then both of them proceed to copy the file. I don't think this is solvable without a file lock, but that means we have to acquire a file lock for every file, unless we can somehow know beforehand which files might be duplicates. This will give a performance hit on all installs, which I guess might not be acceptable.

It's also a problem for creating the tests, since we can still get hit by the original race condition. Annoyingly we hit the race condition pretty often in my test case, since the wheels only have two files. I'm not sure what you would like here. I can add the test still, and make it succeed even if we hit the race condition. That way there issue is a bit more transparent. I can't even make a xfail test, since it will only fail sometimes.

[dimbleby]

perhaps the suggestion here was a good idea all along

[StefanBRas]

perhaps the suggestion here was a good idea all along

Seems like it yes. I'll try it out and see what the performance hit is.

[StefanBRas]

Back with some results:

--------------------------------------------------------------------------------------- benchmark 'demo_wheel': 2 tests ----------------------------------------------------------------------------------------
Name (time in us)                       Min                   Max                  Mean              StdDev              Median                IQR            Outliers         OPS            Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_bench_installer_demo          586.2400 (1.0)        953.1510 (1.0)        644.8547 (1.0)       62.1620 (1.0)      619.0710 (1.0)      66.8820 (1.03)       137;62  1,550.7369 (1.0)        1000           1
test_bench_installer_copy_demo     864.9630 (1.48)     3,915.2940 (4.11)     1,036.5541 (1.61)     223.2947 (3.59)     995.4640 (1.61)     64.6805 (1.0)         24;54    964.7350 (0.62)       1000           1
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

---------------------------------------------------------------------------------------- benchmark 'django_wheel': 2 tests -----------------------------------------------------------------------------------------
Name (time in us)                         Min                   Max                  Mean              StdDev                Median                IQR            Outliers         OPS            Rounds  Iterations
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_bench_installer_django          579.5520 (1.0)      1,111.0400 (1.0)        644.9192 (1.0)       70.9897 (1.0)        616.8720 (1.0)      74.2690 (1.29)       133;54  1,550.5818 (1.0)        1000           1
test_bench_installer_copy_django     879.6270 (1.52)     3,887.0640 (3.50)     1,048.6201 (1.63)     214.2681 (3.02)     1,007.6535 (1.63)     57.5415 (1.0)         33;76    953.6342 (0.62)       1000           1
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Legend:
  Outliers: 1 Standard Deviation from Mean; 1.5 IQR (InterQuartile Range) from 1st Quartile and 3rd Quartile.
  OPS: Operations Per Second, computed as 1 / Mean

This benchmarkes the original implementation versus an implementation that creates a temporary file and then uses os.replace to replace it. One benchmark uses the demo wheel used in the other tests, another uses django because I wanted something with more files.
I included the benchmarking code in the latest commit, but will of course remove it. It's just so you could look at the benchmark setup.

Is this acceptable? I don't know how large a part of actual installation part this is.

[dimbleby]

am I reading right that the units there are microseconds? I would find it hard to get excited over a few hundred microseconds

(though you didn't say what it was that you are measuring. Is that per-file? per-wheel? something else? maybe a millisecond per file would be approaching noticeable...)

[StefanBRas]

I accidentally used the demo wheel in both, here are the actual results:

It's testing how long

WheelInstaller(env, wheel_destination_class=WheelDestination).install(wheel_path)

takes, so it's the time to copy and hash all files from a source wheel to the destination.
The tests with copy in the name (yes, not a great naming convention) is the the proposed implementation that saves to a temporary file first and the does os.replace(...)