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

boost::filesystem::remove Access is denied on windows #216

Closed
mathisloge opened this issue Nov 17, 2021 · 10 comments
Closed

boost::filesystem::remove Access is denied on windows #216

mathisloge opened this issue Nov 17, 2021 · 10 comments

Comments

@mathisloge
Copy link

mathisloge commented Nov 17, 2021

General info

  • Version: 1.77
  • Enviroment: Github Actions
  • Windows server 2019 & windows server 2022
  • c++14

Description

I'm currently adding github actions testing workflow for mapnik and while doing so, I'm experiencing boost::filesystem::remove: Access is denied error. I've replaced boost::filesystem::remove with c++20 std::filesystem::remove and that worked and don't had an access denied error. I've also added a simple std::this_thread::sleep_for(1s) before the remove to let any other processes from the creation release a potential file access. But that didn't work either.

On my local machines (windows 10 & windows 11) there is no such problem and because this is on a CI enviroment (github actions) where I don't have a deep access to debugging is very hard. Ubuntu and macos are working both in local and CI enviroment.

The file itself is created while running the tests and should be removed after the test run. The file does exists (I've downloaded the whole build dir from the CI).

I've created a github virtual enviroment repo discussion to ask them if it is something with the image. I have added TAKEOWN \F <dir> but that didn't helped.

Is there something I can do, to narrow the problem down and fix it?

useful links:

@mathisloge
Copy link
Author

This might be a related PR in the microsoft stl lib microsoft/STL#1559

@Lastique
Copy link
Member

If this is indeed related to read-only attribute, you can show the attribute in the GHA log by executing attrib command. Please confirm that this is the actual source of the problem. If not, I will need a small self-contained repro.

@mathisloge
Copy link
Author

So i've tried to create a repro that maps the flow in a simple way in https://github.com/mathisloge/boost-repro
However I can't reproduce the same error as in mapnik.

I've used attrib to get the flag. But that looks like it has all the needed attibutes:
A D:\a\mapnik\mapnik\build\windows-ci\out\test\data\shp\boundaries.index

Full:

      Start 19: invalid shapeindex
19/87 Test #19: invalid shapeindex .......................................***Failed    0.06 sec
A                    D:\a\mapnik\mapnik\build\windows-ci\out\test\data\shp\boundaries.index
Filters: invalid shapeindex

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mapnik-test-unit.exe is a Catch v2.13.7 host application.
Run with -? for options

-------------------------------------------------------------------------------
invalid shapeindex
  Invalid index
-------------------------------------------------------------------------------
D:\a\mapnik\mapnik\test\unit\datasource\shapeindex.cpp(102)
...............................................................................

D:\a\mapnik\mapnik\test\unit\datasource\shapeindex.cpp(102): FAILED:
  {Unknown expression after the reported line}
due to unexpected exception with message:
  boost::filesystem::remove: Access is denied: "test/data/shp/boundaries.index"

I'll keep trying to reproduce the error.

Lastique added a commit that referenced this issue Nov 17, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Closes #216.
Lastique added a commit that referenced this issue Nov 17, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Closes #216.
Lastique added a commit that referenced this issue Nov 17, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

As a side effect, we were able to remove an implementation detail value from
the file_type enum that was used by the old remove() implementation.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Closes #216.
@Lastique
Copy link
Member

I have implemented support for deleting read-only files in branch feature/windows_remove_read_only. Could you test if it resolves your issue?

@mathisloge
Copy link
Author

mathisloge commented Nov 17, 2021

wow, that was pretty fast. A D:\a\mapnik\mapnik\build\windows-ci\out\test\data\shp\boundaries.index had the correct attributes. I'm curious to see if it solves the problem anyway. The CI is running and might take a hour. Will report tomorrow.
I also added some other debug output in two other runs to narrow it further down.

If the run fails with the file system change, I will somehow try to debug it further before you put more effort into it. Thanks!

Lastique added a commit that referenced this issue Nov 17, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

As a side effect, we were able to remove an implementation detail value from
the file_type enum that was used by the old remove() implementation.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Also, corrected mklink /J argument in tests. The command accepts /j (lowercase)
to the same effect, but the formal help lists /J (uppercase) to create junctions.

Closes #216.
Lastique added a commit that referenced this issue Nov 17, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

As a side effect, we were able to remove an implementation detail value from
the file_type enum that was used by the old remove() implementation.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Also, corrected mklink /J argument in tests. The command accepts /j (lowercase)
to the same effect, but the formal help lists /J (uppercase) to create junctions.

Related to #216.
@mathisloge
Copy link
Author

So I've runned with the feature branch. But unfortunately it doesn't resolve my issue.
In the current runs, I do a side-by-side comparison by replacing boost::filesystem::remove with the stl equivalent. Both compiled as c++20 so as not to get any stl side effects from the version as there might be in the previous attemps.

@Lastique
Copy link
Member

Lastique commented Nov 18, 2021

In that case I need a repro.

My guess is that the file is opened (by the same or a different process) without the FILE_SHARE_DELETE flag, and this prevents the file from being deleted via DeleteFileW. MSVC standard library uses a different, not fully documented way of deleting files that enables POSIX semantics of file deletion, which I guess is not affected by this (I don't know if this is indeed the case). This feature only appeared in recent Windows 10 versions, and Boost.Filesystem supports older Windows versions as well, so I won't be able to use the same approach universally. Which means that your test will fail on older Windows versions regardless of filesystem::remove implementation.

I probably will implement the POSIX removal semantics on Windows at some point, but it will have to be opportunistic, and therefore will not be trivial. In any case, this will not be done in the immediate future.

Lastique added a commit that referenced this issue Nov 18, 2021
Reworked remove() operation to separate POSIX and Windows implementations.
On Windows, if the file to be removed is read-only, try to reset the read-only
attribute before deleting the file. If deleting fails (other than because the
file is already deleted), try to restore the read-only attribute.

As a side effect, we were able to remove an implementation detail value from
the file_type enum that was used by the old remove() implementation.

Added a test for remove() on a read-only file on Windows. Also added tests
for remove_all(), including for cases with symlinks, hardlinks and read-only
files.

Also, corrected mklink /J argument in tests. The command accepts /j (lowercase)
to the same effect, but the formal help lists /J (uppercase) to create junctions.

Reported in #216.
@mathisloge
Copy link
Author

mathisloge commented Nov 18, 2021

My guess is that the file is opened

think so too. The lib uses memory mapped files to access other files. After disabling this the tests are running successful.
The question is why this happens only on the CI system but not on my local systems (CI is running windows-server 2019 and 2022).

recent Windows 10 versions

maybe this is not implemented in windows server enviroments, too?

I will try to create a repro for your future use

@Lastique
Copy link
Member

maybe this is not implemented in windows server enviroments, too?

I don't know how Windows server editions correspond to desktop ones. The POSIX file delete semantics is reportedly implemented since Windows 10 1709.

It may well be something else entirely.

@mathisloge
Copy link
Author

So yeah this

My guess is that the file is opened

is correct. There seems to be differences between windows desktop and windows server enviroments. If I'm running the test case of https://github.com/mathisloge/boost-repro (currently a bit too much code, I make it simpler) on my windows desktop it is successful and if I'm running it on the CI (windows server 2022) it will fail.

Lastique added a commit that referenced this issue Mar 1, 2022
Windows 10 1709 and later support POSIX semantics for removing files,
which means the file name is removed from the filesystem namespace as
soon as the file is marked for deletion. This makes opening the file
afterwards impossible, and allows creating a new file with the same
name, even if the deleted file is still open and in use.

The implementation uses runtime detection of the feature in the OS.
We are also using two more implementations for file removal: one that
employs the more recent FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE
flag (available since Windows 10 1809), and FILE_DISPOSITION_INFO
structure (supported since Windows Vista). The former allows to optimize
removal of read-only files, and the latter allows to make file deletion
atomic (i.e. not prone to failure if the file is replaced on the filesystem
while the operation is executing). The implementation is chosen in
runtime, depending on which one succeeds removing a file.

Also, added support for deleting read-only directories, in addition
to non-directory files, and simplified code a little.

Closes #216.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants