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

Fix caching on windows #178

Merged
merged 6 commits into from
Dec 6, 2021
Merged

Fix caching on windows #178

merged 6 commits into from
Dec 6, 2021

Conversation

woodruffw
Copy link
Member

Closes #169.

@schlenk or @matthewdeanmartin: could you give this a spin and see if it improves the caching situation on your hosts?

@woodruffw woodruffw self-assigned this Dec 3, 2021
@woodruffw woodruffw added plat:windows Issues reported on Windows performance Something isn't as fast or responsive as it should be. labels Dec 3, 2021
@schlenk
Copy link

schlenk commented Dec 4, 2021

Gave it a try, but did not solve the issue. I took a closer look with ProcessMonitor, to see which syscalls happend and the virus scanner does open the files, but is not really interfering in this case.

The actual issue why the writes failed is this:
https://stackoverflow.com/questions/7147577/programmatically-rename-open-file-on-windows
(see also https://devblogs.microsoft.com/oldnewthing/20211022-00/?p=105822 )

You try to rename the file while INSIDE the with NamedTemporaryFile() block, so the file handle is still open at the time the rename/replace() happens. This leads to a SHARING_VIOLATION and the replace() fails.

NamedTemporaryFile with the O_TEMPORARY flag actually sets SHARE_DELETE, but that would not help, as it deletes the file on close, which is not what you really want here. And python does not provide a simple way to open a file with share mode delete ( you can do it with a bit of ctypes, CreateFile, os.openfd() & mscvrt.open_osfhandle(), but it is a bit clumsy.).

So one way that works (on windows) is to put the os.replace() outside the with block, so it happens after the file is closed.

@matthewdeanmartin
Copy link

And python does not provide a simple way to open a file with share mode delete ( you can do it with a bit of ctypes, CreateFile, os.openfd() & mscvrt.open_osfhandle(), but it is a bit clumsy.).

While hacking on an a tangentially related problem and I ran into that same temporary file bug on windows and stack overflow suggest this exact solution so that python temp files would work on windows, especially if someone is trying to do something fancy with them before the close:

https://github.com/matthewdeanmartin/caniuseonlywheels/blob/master/test/custom_temp_file.py

@woodruffw
Copy link
Member Author

Thanks for digging into that! Yeah, that makes more sense and should be much simpler to test. I'll update the PR accordingly.

@woodruffw
Copy link
Member Author

Okay, this should be good for another round of tests. Could one of you give it a spin again?

@schlenk
Copy link

schlenk commented Dec 6, 2021

Works for me now. No more warnings and the cache gets populated and is reused on a second run.

@woodruffw
Copy link
Member Author

Awesome!

@woodruffw woodruffw requested a review from di December 6, 2021 17:19
@woodruffw woodruffw merged commit a196f05 into main Dec 6, 2021
@woodruffw woodruffw deleted the ww/retry-rename-windows branch December 6, 2021 17:24
@di di changed the title pypi: add a retry loop for cache updates Fix caching on windows Dec 6, 2021
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 7, 2021
## [1.1.0]

### Added

* CLI: The `--path <PATH>` flag has been added, allowing users to limit
  dependency discovery to one or more paths (specified separately)
  when `pip-audit` is invoked in environment mode
  ([#148](pypa/pip-audit#148))

* CLI: The `pip-audit` CLI can now be accessed through `python -m pip_audit`.
  All functionality is identical to the functionality provided by the
  `pip-audit` entrypoint
  ([#173](pypa/pip-audit#173))

* CLI: The `--verbose` flag has been added, allowing users to receive more
  more verbose output from `pip-audit`. Supplying the `--verbose` flag
  overrides the `PIP_AUDIT_LOGLEVEL` environment variable and is equivalent to
  setting it to `debug`
  ([#185](pypa/pip-audit#185))

### Changed

* CLI: `pip-audit` now clears its spinner bar from the terminal upon
  completion, preventing visual confusion
  ([#174](pypa/pip-audit#174))

### Fixed

* Dependency sources: a crash caused by `platform.python_version` returning
  an version string that couldn't be parsed as a PEP-440 version was fixed
  ([#175](pypa/pip-audit#175))

* Dependency sources: a crash caused by incorrect assumptions about
  the structure of source distributions was fixed
  ([#166](pypa/pip-audit#166))

* Vulnerability sources: a performance issue on Windows caused by cache failures
  was fixed ([#178](pypa/pip-audit#178))

## [1.0.1] - 2021-12-02

### Fixed

* CLI: The `--desc` flag no longer requires a following argument. If passed
  as a bare option, `--desc` is equivalent to `--desc on`
  ([#153](pypa/pip-audit#153))

* Dependency resolution: The PyPI-based dependency resolver no longer throws
  an uncaught exception on package resolution errors; instead, the package
  is marked as skipped and an appropriate warning or fatal error (in
  `--strict` mode) is produced
  ([#162](pypa/pip-audit#162))

* CLI: When providing the `--cache-dir` flag, the command to read the pip cache
  directory is no longer executed. Previously this was always executed and
  could result into failure when the command fails. In CI environments, the
  default `~/.cache` directory is typically not writable by the build user and
  this meant that the `python -m pip cache dir` would fail before this fix,
  even if the `--cache-dir` flag was provided.
  ([#161](pypa/pip-audit#161))

## [1.0.0] - 2021-12-01

### Added

* This is the first stable release of `pip-audit`! The CLI is considered
  stable from this point on, and all changes will comply with
  [Semantic Versioning](https://semver.org/)

## [0.0.9] - 2021-12-01

### Added

* CLI: Skipped dependencies are now listed in the output of `pip-audit`,
  for supporting output formats
  ([#145](pypa/pip-audit#145))
* CLI: `pip-audit` now supports a "strict" mode (enabled with `-S` or
  `--strict`) that fails if the audit if any individual dependency cannot be
  resolved or audited. The default behavior is still to skip any individual
  dependency errors ([#146](pypa/pip-audit#146))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Something isn't as fast or responsive as it should be. plat:windows Issues reported on Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frequent cache access error warnings on Win32
4 participants