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

Windows: retry a file remove - a virus checker might lock files #11437

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −1
Open

Windows: retry a file remove - a virus checker might lock files #11437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion otherlibs/stdune/src/fpath.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,13 @@ let win32_unlink fn =
Unix.chmod fn 0o666;
Unix.unlink fn
with
| _ -> raise e)
| _ ->
(* On Windows a virus scanner frequently has a lock on new executables for a short while - just retry *)
let rec retry_loop cnt =
Unix.sleep 1;
(try Unix.unlink fn
with _ -> if cnt>0 then retry_loop (cnt-1) else raise e)
in retry_loop 30)
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Should we restrict this logic only if Sys.win32 ?
  • Should we print a message to the console to inform the user of what is going on? Something like Could not delete file %S. Retrying...

Copy link
Author

Choose a reason for hiding this comment

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

As the name of the function implies it is Windows specific - this function is not called unless we have Sys.win32.

I would say we should definitely print a message in verbose mode, but I would need advice how to do it - I expect that dune has some elaborate logging facilities which should be used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the name of the function implies it is Windows specific - this function is not called unless we have Sys.win32.

Right, thanks.

I would say we should definitely print a message in verbose mode, but I would need advice how to do it - I expect that dune has some elaborate logging facilities which should be used.

I think that the Log module is what you should use. Grep for Log.info to see examples of its use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say yes to both questions.

Copy link
Author

Choose a reason for hiding this comment

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

@maiste : as discussed, this code is already WIndows specfic, so there is no point in checking for Sys.win32.

I will look into adding an info message.

Copy link
Collaborator

@maiste maiste Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, sorry, I wrote my replies without reloading the page (only see first reply at this time). Thanks! 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a bit worried about blocking Dune for that long (up to 30x 1 second), as that will lock the entirety of the Dune execution and not even other processes can execute.

Maybe then it should be converted to a Fiber?

;;

let unlink_exn = if Stdlib.Sys.win32 then win32_unlink else Unix.unlink
Expand Down
Loading