Fix OS.Dir.delete on Linux and Windows.

[dbuenzli]

A call to Unix.unlink on a directory returns EISDIR on Linux (it seems to be EPERM on osx), so the function here fails. Maybe we should rather properly stat the thing before, it seems osx is collapsing different errors in a single one.

[dbuenzli]

it seems osx is collapsing different errors in a single one.

In fact osx is POSIX compliant, EISDIR is Linux specific.

[dbuenzli]

Gah I tried with @fdopen's OCaml64 and it seems to return EACCESS on directories. Would it work if I check for EACCESS on Sys.win32 ? Or are there other error conditions that will return this error (I'm trying to avoid useless stat by detecting a dir depending on how unlink fails).

[dsheets]

How about rmdir first with a check for ENOTDIR followed by unlink?

[dsheets]

Slower in the common case there, I suppose. I don't see your retry logic but I would suggest just attempting the operations you want to execute and checking errnos rather than using something like stat which will be racy.

[fdopen]

The windows version of Unix.unlink still uses the obsolete crt library (and Unix.rmdir, too). It won't return any useful error codes at all. EACCES is the only possible error code mentioned beside ENOENT: https://msdn.microsoft.com/en-us/library/1c3tczd6.aspx

[dbuenzli]

How about rmdir first with a check for ENOTDIR followed by unlink?

That could be an idea. But given what @fdopen says I think I'll just check for EACCESS when Sys.win32.

I don't see your retry logic but I would suggest just attempting the operations you want to execute and checking errnos rather than using something like stat which will be racy.

The thing is here. The whole thing is already incredibly racy in the sense that to "rmdir -r" a directory I first delete all the files in the hiearchy with unlink using its return code to detect what directories are and then remove all the directories using rmdir (starting from the leaves).

[dsheets]

Because there are multiple operations occurring in succession and POSIX has nothing like transactions for file system operations, any implementation will be racy to some extent. The point I am trying to make (and which I think you probably already understand) is that a robust solution shouldn't assume that because a prior operation succeeded or returned one value that a later one will behave as expected. This is why I recommend using error codes in response to actions (potentially mutually recursively retrying) rather than querying and then relying on the results of the query to be accurate.

I think your retry logic makes sense in general. The only surprising race I see is if one uses your function to recursively delete while another process is creating files or directories in the hierarchy. I believe, in that case, your function will error with something like ENOTEMPTY during the rmdir phase when I would expect it to continue attempting deletion. Perhaps this is not a behavior you want or a use case you care about but I would certainly find it surprising if delete can error with ENOTEMPTY.

[dbuenzli]

This is why I recommend using error codes in response to actions (potentially mutually recursively retrying) rather than querying and then relying on the results of the query to be accurate.

Yes that seems good advice and what the code does here (though I didn't do it knowingly).

I believe, in that case, your function will error with something like ENOTEMPTY during the rmdir phase when I would expect it to continue attempting deletion. Perhaps this is not a behavior you want or a use case you care about but I would certainly find it surprising if delete can error with ENOTEMPTY.

You are correct and it would be surprising. But OTOH you could trick programs using the function into infinite loops which could lead to DOS so I think I'll keep it that way for now.

[dsheets]

But OTOH you could trick programs using the function into infinite loops which could lead to DOS so I think I'll keep it that way for now.

This DoS would require another program that is able to create file and directories that is also in an infinite loop. I don't see that as a security threat at all. A reliable recursive deletion function is useful.

[dbuenzli]

This DoS would require another program that is able to create file and directories that is also in an infinite loop.

Well writing a program that is in an infinite loop is not a problem, but I agree the actual DoS would be racy. I discussed this with @pqwy and he suggested to simply add a retries:int optional argument. If unspecified it tries infinitely often until the directory is gone, otherwise it will only retry the specified number of times.

[dbuenzli]

So the code now works on both linux and windows, I'm closing this. I have opened #54 to improve the reliablity of the function as @dsheets suggest and according to https://github.com/dbuenzli/bos/issues/50#issuecomment-223592710.