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

Use CREATE_ALWAYS instead of CREATE_NEW #120

Closed
wants to merge 2 commits into from

Conversation

cloudwu
Copy link
Contributor

@cloudwu cloudwu commented Nov 14, 2018

CREATE_ALWAYS is better than CREATE_NEW for dir locking IMHO, because lockfile.lfs may be not deleted in some cases.

@coveralls
Copy link
Collaborator

Coverage Status

Coverage increased (+0.1%) to 80.244% when pulling 1f298b1 on cloudwu:patch-1 into 1dfb8c4 on keplerproject:master.

@coveralls
Copy link
Collaborator

coveralls commented Nov 14, 2018

Coverage Status

Coverage remained the same at 82.227% when pulling cea32e6 on cloudwu:patch-1 into 6048863 on keplerproject:master.

@hishamhm
Copy link
Member

@cloudwu I'm not too familiar with the Windows API, but from what I'm reading here my understanding is that the use of CREATE_NEW is deliberate, in order to fail in case a lockfile from another concurrent execution already exists. If we used CREATE_ALWAYS here, then two concurrent executions would be able to acquire the lock.

I'm closing this for now, but feel free to reopen if my understanding is not correct.

@hishamhm hishamhm closed this Apr 21, 2020
@cloudwu
Copy link
Contributor Author

cloudwu commented Apr 21, 2020

The exclusive here is guaranteed by GENERIC_WRITE rather than the existence of the file "lockfile.lfs" . windows doesn't allow two or more concurrent execution open the file with GENERIC_WRITE at the same time.

It's easier to fail to delete the file "lockfile.lfs" in windows, although we set FILE_FLAG_DELETE_ON_CLOSE. I found many times my app can't delete the file "lockfile.lfs" in windows after exit , so I think we should change CREATE_NEW to CREATE_ALWAYS (or OPEN_ALWAYS) . The lock can still work whether we delete "lockfile.lfs" last time.

@hishamhm hishamhm reopened this Apr 21, 2020
@hishamhm
Copy link
Member

@cloudwu Thank you for the clarification! I fixed the conflict and merged your commit manually!

@hishamhm hishamhm closed this Apr 21, 2020
@cloudwu cloudwu deleted the patch-1 branch April 22, 2020 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants