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.

Sign up for GitHub

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

Jump to bottom

fix: pass O_SHARE_DELETE when digesting files #8262

Merged
merged 1 commit into from
Jul 27, 2023
Merged

fix: pass O_SHARE_DELETE when digesting files #8262

merged 1 commit into from
Jul 27, 2023

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jul 24, 2023 •
edited
Loading

Fixes #8268

This flag corresponds to what Stdlib.open_in does. When passed, the files can be removed while they are still open. This condition can happen when background digests are enabled. This is a no-op everywhere else.

This allows enabling background digests on Windows.

@emillon emillon mentioned this pull request Jul 24, 2023
Closed
rgrinberg
rgrinberg reviewed Jul 24, 2023
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Can we leave the old code on Unix? Channels have mutexes that we don't really need here

@emillon
Copy link
Collaborator Author

emillon commented Jul 25, 2023

I'll try the other following approaches:

  • use Unix.open with _O_NOINHERIT
  • replace close by CloseHandle(Handle_val(v))

@emillon
Copy link
Collaborator Author

emillon commented Jul 25, 2023

Ah, even better: adding O_SHARE_DELETE at the open point fixes the issue (and is a noop on nonwindows). I'll update the PR.

@emillon emillon changed the title WIP: fix: dune_digest: don't open with Unix module fix: pass O_SHARE_DELETE when digesting files Jul 25, 2023
@emillon emillon marked this pull request as ready for review July 25, 2023 13:16
@emillon
Copy link
Collaborator Author

emillon commented Jul 25, 2023

There's still a part I don't completely understand (why the file is closed too late in some cases) but it might be related to the details of threading and win32unix.

@emillon emillon requested a review from rgrinberg July 25, 2023 13:17
@rgrinberg rgrinberg requested a review from nojb July 25, 2023 13:41
nojb
nojb approved these changes Jul 25, 2023
View reviewed changes
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM. Understanding why the file is not closed in the first place would be ideal of course, but this fix looks solid.

@emillon
Copy link
Collaborator Author

emillon commented Jul 26, 2023

I added a comment explaining the situation. let me know what you think.

@emillon
fix: pass O_SHARE_DELETE when digesting files
86d940c 
Fixes ocaml#8268

This flag corresponds to what `Stdlib.open_in` does. When passed, the
files can be removed while they are still open. This condition can
happen when background digests are enabled. This is a no-op everywhere
else.

This allows enabling background digests on Windows.

Signed-off-by: Etienne Millon <me@emillon.org>
@nojb
Copy link
Collaborator

nojb commented Jul 26, 2023

I added a comment explaining the situation. let me know what you think.

LGTM

@hhugo
Copy link
Collaborator

hhugo commented Jul 26, 2023

LGTM

@emillon emillon merged commit d8bd6fa into ocaml:main Jul 27, 2023
18 of 19 checks passed
@emillon emillon deleted the fix-windows-open branch July 27, 2023 08:25
@emillon emillon mentioned this pull request Jul 28, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 28, 2023
@emillon
[new release] dune (15 packages) (3.10.0~alpha1)
9ddddfc 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 31, 2023
@emillon
[new release] dune (15 packages) (3.10.0)
82e64cd 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
@emillon emillon mentioned this pull request Jul 31, 2023
Merged
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
@emillon
fix: pass O_SHARE_DELETE when digesting files (ocaml#8262)
1630488 
Fixes ocaml#8268

This flag corresponds to what `Stdlib.open_in` does. When passed, the
files can be removed while they are still open. This condition can
happen when background digests are enabled. This is a no-op everywhere
else.

This allows enabling background digests on Windows.

Signed-off-by: Etienne Millon <me@emillon.org>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
@emillon
fix: pass O_SHARE_DELETE when digesting files (ocaml#8262)
c78ac88 
Fixes ocaml#8268

This flag corresponds to what `Stdlib.open_in` does. When passed, the
files can be removed while they are still open. This condition can
happen when background digests are enabled. This is a no-op everywhere
else.

This allows enabling background digests on Windows.

Signed-off-by: Etienne Millon <me@emillon.org>
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
[new release] dune (15 packages) (3.10.0)
5407dc7 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hhugo hhugo hhugo left review comments

@nojb nojb nojb approved these changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

background digests crash on Windows
4 participants
@emillon @nojb @hhugo @rgrinberg