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: disable background digests on Windows #8243

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jul 21, 2023

Fixes #8228

In some cases when the shared cache is enabled, some temporary directories cannot be removed.

@emillon
Copy link
Collaborator Author

emillon commented Jul 21, 2023

We can probably come up with a better fix but this one would mitigate the issue.

@nojb
Copy link
Collaborator

nojb commented Jul 21, 2023

This fix is not great; it will leave temporary directories and files laying around that will never be cleaned up. Do you understand when is the rm_rf called? Could the issue be solved by delaying this call somehow until other threads are done? For example, in the Temp module of stdune we register an exit hook to clean all temporary files and directories; could that be relevant here? (I haven't looked at the code, so maybe this is precisely the context in which the bug is triggered...)

@Alizter
Copy link
Collaborator

Alizter commented Jul 23, 2023

I don't understand how we run into this issue in the first place. When creating temporary directories in Windows, it is my understanding that we ask windows for a unique temp directory name which gets put into the common temp dir location. Then we can do whatever we like with the files inside and finally delete the temp directory since the name is unqiue.

It appears in this case we are placing a non-unique directory name (from the caching stuff?) directly ourselves into the common windows temp dirs and then getting race conditions when two processes touch the same name. Wouldn't the solution be to fix how we create and delete temp directories in windows rather than ignoring the errors?

@rgrinberg
Copy link
Member

How about we just disable background digests on windows until we understand the issue better?

@emillon
Copy link
Collaborator Author

emillon commented Jul 24, 2023

How about we just disable background digests on windows until we understand the issue better?

Sure, that's an even better workaround. I'll change the PR to do that and add that to 3.9.2.

@emillon emillon requested a review from rgrinberg July 24, 2023 15:38
@emillon emillon changed the title fix: in Fpath.rm_rf, ignore ENOTEMPTY on Windows fix: disable background digests on Windows Jul 25, 2023
@emillon
Copy link
Collaborator Author

emillon commented Jul 25, 2023

done

This was referenced Jul 25, 2023
Fixes ocaml#8228

In some cases when the shared cache is enabled, some temporary
directories cannot be removed.

Signed-off-by: Etienne Millon <[email protected]>
; value = background_default
; value =
(match Platform.OS.value with
| Linux -> `Enabled
Copy link
Member

Choose a reason for hiding this comment

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

Why did you disable it on Mac and the rest of the unixes?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I remember that we have that fork bug as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can introduce a Toggle.and_ to express that as background_default and not windows if you think it's more explicit.

@emillon emillon merged commit 979cd2c into ocaml:main Jul 25, 2023
@emillon emillon deleted the fix-8228 branch July 25, 2023 09:38
emillon added a commit to emillon/dune that referenced this pull request Jul 25, 2023
Fixes ocaml#8228

In some cases when the shared cache is enabled, some temporary
directories cannot be removed.

Signed-off-by: Etienne Millon <[email protected]>
emillon added a commit that referenced this pull request Jul 25, 2023
Fixes #8228

In some cases when the shared cache is enabled, some temporary
directories cannot be removed.

Signed-off-by: Etienne Millon <[email protected]>
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 25, 2023
CHANGES:

- Disable background digests on Windows. This prevents an issue where
  unremovable files would make dune crash when the shared cache is enabled.
  (ocaml/dune#8243, fixes ocaml/dune#8228, @emillon)

- Fix permission errors when `sendfile` is not available (ocaml/dune#8234, fixes ocaml/dune#8120,
  @emillon)
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
Fixes ocaml#8228

In some cases when the shared cache is enabled, some temporary
directories cannot be removed.

Signed-off-by: Etienne Millon <[email protected]>
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Disable background digests on Windows. This prevents an issue where
  unremovable files would make dune crash when the shared cache is enabled.
  (ocaml/dune#8243, fixes ocaml/dune#8228, @emillon)

- Fix permission errors when `sendfile` is not available (ocaml/dune#8234, fixes ocaml/dune#8120,
  @emillon)
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.

Issue building packages on windows: rmdir, Directory not empty
4 participants