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

Windows watch mode: fix a small race condition, re-enable tests #7190

Merged
merged 6 commits into from
Mar 23, 2023

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Feb 27, 2023

This PR contains a small fix for the Windows watch mode #7010 that can result in some notifications being lost when they arrive immediately after the watched directory is added. It also re-enables a unit test for this watcher backend.

nojb added 2 commits February 26, 2023 22:27
Signed-off-by: nojebar <[email protected]>
@rgrinberg rgrinberg added this to the 3.8.0 milestone Feb 27, 2023
@rgrinberg rgrinberg requested a review from jonahbeckford March 11, 2023 06:20
@rgrinberg
Copy link
Member

@MisterDA could you take a look?

@nojb
Copy link
Collaborator Author

nojb commented Mar 11, 2023

Description of the changes for the prospective reviewer:

  1. when adding a new directory, the OCaml thread sends a message to the native thread to do the actual addition. This means that there is a delay between calling Fswatch_win.add and the actual start of the monitoring. This can cause some events to be missed if the events arrive very quickly after calling Fswatch_win.add (this was the case in the unit test). To fix this, we make sure that Fswatch_win.add does not return until monitoring is actually started in the other thread. We communicate this fact using an event object (afterAdd).
  2. For the same reason, it is not a good idea to pass relative paths to Fswatch_win.add (as the current directory could change between the call to Fswatch_win.add and the actual point in time when monitoring starts). Of course with 1) this should not happen, but for extra safety I enforce that directories passed to Fswatch_win.add be absolute (this is the case in current Dune codebase).

@jonahbeckford
Copy link
Collaborator

Just saw this. Will have a review tomorrow.

Copy link
Collaborator

@jonahbeckford jonahbeckford left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks.

@nojb nojb merged commit b5487f0 into ocaml:main Mar 23, 2023
@nojb nojb deleted the fswatch_win_fix branch March 23, 2023 01:54
@nojb
Copy link
Collaborator Author

nojb commented Apr 3, 2023

@emillon this one can go in 3.7.1.

emillon pushed a commit to emillon/dune that referenced this pull request Apr 3, 2023
@emillon
Copy link
Collaborator

emillon commented Apr 3, 2023

done in #7474

emillon pushed a commit to emillon/dune that referenced this pull request Apr 4, 2023
emillon added a commit that referenced this pull request Apr 4, 2023
… (#7474)

Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Co-authored-by: Nicolás Ojeda Bär <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants