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

Clarify file sharing flags in FML filesystem APIs on Windows #38164

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

jason-simmons
Copy link
Member

Use shared mode when requesting read access and exclusive mode for write access

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jason-simmons
Copy link
Member Author

This may be related to the errors that ImpellerC is sometimes getting on LUCI when it opens the build output directory (see flutter/flutter#116761)

@chinmaygarde
Copy link
Member

Not sure how to interpret this TBH. Why should the file be allowed to be written or deleted as we access it? The exclusive access bit makes sense though.

@jason-simmons
Copy link
Member Author

Currently the Windows implementation of FilePermission::kRead is specifying only FILE_SHARE_READ. This means that Windows will refuse to open the file/directory if some other process has opened it for write access
(see https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea)

This seems too restrictive. For something like ImpellerC's use of fml::OpenDirectory it is unnecessary to require that level of exclusivity just to get a read-only handle to a directory. I suspect the same applies to other file/directory reading scenarios.

FML is not applying this kind of locking for file access on POSIX platforms.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

LGTM as a speculative fix. Not sure how to test this but lets timebox something?

@jason-simmons
Copy link
Member Author

Added a test to verify that fml::OpenDirectory(FilePermission::kRead) works if the directory has also been opened for writing on Windows

Use shared mode when requesting read access and exclusive mode for write access
@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 13, 2022
@auto-submit auto-submit bot merged commit 9872cc7 into flutter:main Dec 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 13, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 13, 2022
…116997)

* f863b1978 Roll Skia from c83eef7dc2a3 to 971c342c3030 (2 revisions) (flutter/engine#38248)

* 9872cc7ad Clarify file sharing flags in FML filesystem APIs on Windows (flutter/engine#38164)
loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Dec 16, 2022
…#38164)

Use shared mode when requesting read access and exclusive mode for write access
loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023
…#38164)

Use shared mode when requesting read access and exclusive mode for write access
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…lutter#116997)

* f863b1978 Roll Skia from c83eef7dc2a3 to 971c342c3030 (2 revisions) (flutter/engine#38248)

* 9872cc7ad Clarify file sharing flags in FML filesystem APIs on Windows (flutter/engine#38164)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants