crystal icon indicating copy to clipboard operation
crystal copied to clipboard

`File#delete` method does not work on windows

Open Blacksmoke16 opened this issue 2 years ago • 3 comments

tempfile = File.tempfile "foo", ".bar"
tempfile.delete
Unhandled exception: Error deleting file: 'C:\\Users\\Georg\\AppData\\Local\\Temp/foo.da26ef3085d894dfcd12d7130c43dd03.bar': Permission denied (File::AccessDeniedError)
  from C:\Users\Georg\dev\git\crystal\src\crystal\system\win32\file.cr:173 in 'delete:raise_on_missing'
  from C:\Users\Georg\dev\git\crystal\src\file.cr:363 in 'delete'
  from C:\Users\Georg\dev\git\crystal\src\file.cr:904 in 'delete'
  from test.cr:2 in '__crystal_main'
  from C:\Users\Georg\dev\git\crystal\src\crystal\main.cr:115 in 'main_user_code'
  from C:\Users\Georg\dev\git\crystal\src\crystal\main.cr:101 in 'main'
  from C:\Users\Georg\dev\git\crystal\src\crystal\main.cr:127 in 'main'
  from C:\Users\Georg\dev\git\crystal\src\crystal\system\win32\wmain.cr:35 in 'wmain'
  from D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288 in '__scrt_common_main_seh'
  from C:\WINDOWS\System32\KERNEL32.DLL +94260 in 'BaseThreadInitThunk'
  from C:\WINDOWS\SYSTEM32\ntdll.dll +337489 in 'RtlUserThreadStart'

Based on https://stackoverflow.com/a/62997498, it seems this is because Windows prevents deleting the file since it has an open handle to it. I was able to resolve this by adding LibC::O_TEMPORARY to the oflag we pass to https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=msvc-170, but not sure if that's the proper way to go about it given the description made it sound like it's somewhat legacy?

EDIT: This also affects files you have a reference to via File.open. ~~EDIT2: Workaround is to call #close before #delete.~~

Blacksmoke16 avatar Aug 16 '22 01:08 Blacksmoke16

Related ruby issue/commit: https://bugs.ruby-lang.org/issues/11218 / https://github.com/ruby/ruby/commit/2bc2802096252be6b91be8ddbb29a635f9afbd10. Looks like the proper flag to use is FILE_SHARE_DELETE.

EDIT: NVM, seems they're using a diff file API than we are, so the commit isn't relevant. But I guess is there a reason we're using _wopen and CreateFileW?

Blacksmoke16 avatar Aug 16 '22 03:08 Blacksmoke16

Hm, I suppose File#delete just needs to close the file before deleting it on windows?

There's also an alternative function for closing files: DeleteFileW. We could consider using this instead of _wunlink. Not sure why to chose one or the other. But it basically works the same, although the error message is a bit more clear. It states the file can't be deleted because it's in use. The documentation for that function says the file would be marked for deletion on close, but I couldn't observe that. Maybe this only applies when deleting from a different process...

straight-shoota avatar Aug 16 '22 07:08 straight-shoota

Interestingly enough it seems the Ruby API docs call this out, saying:

unlinking a non-closed file will result in an error, which this method will silently ignore

So I suppose that's another way to go about it? Seems they also have a slightly diff API where you can close and delete the temp file via the same method, so it's less likely to forget to close it before deleting it. Also worth pointing out it seems Ruby uses wunlink too for deletions. For opening, my C is a bit rusty, but based on https://github.com/ruby/ruby/blob/58c8b6e86273ccb7a1b903d9ab35956b69b3b1bf/win32/win32.c#L6321-L6490 it seems they use _wopen in one context, but fallback on CreateFileW otherwise.

Ref: https://ruby-doc.org/stdlib-3.1.2/libdoc/tempfile/rdoc/Tempfile.html#method-i-unlink-label-Unlink-before-close

Blacksmoke16 avatar Aug 16 '22 22:08 Blacksmoke16

At the end of the day, anything on win32 subsystem will translate to Win32 C APIs. Why not directly use DeleteFileW and CreateFileW with FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_SHARE_DELETE as needed?

** What Windows Engineer says on it : https://github.com/golang/go/issues/32088#issuecomment-502850674

** How Go lang does it : https://github.com/golang/go/issues/32088#issue-445117894

** https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew

** https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew

mominshaikhdevs avatar Feb 09 '23 07:02 mominshaikhdevs