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

Fix for Issue:1780 #1831

Closed
wants to merge 4 commits into from
Closed

Conversation

stevelittle
Copy link

#1780

Move the check of lockedFiles to before we attempt to open the lock file
This way we avoid opening a file which might already be open.
More importantly, we avoid closing that file, which would release any
fcntl locks that we have, enabling concurrent access from other processes
and potential file corruption

@facebook-github-bot
Copy link
Contributor

@stevelittle updated the pull request - view changes

stevelittle and others added 4 commits February 3, 2017 06:23
This test demonstrates that attempting to take a 2nd lock
within the same process causes the existing fcntl lock to be
dropped
Move the check of lockedFiles to before we attempt to open the lock file
This way we avoid opening a file which might already be open.
More importantly, we avoid *closing* that file, which would release any
fcntl locks that we have, enabling concurrent access from other processes
and potential file corruption
For now, we can just #ifdef them out and assume it works
@facebook-github-bot
Copy link
Contributor

@stevelittle updated the pull request - view changes

@stevelittle
Copy link
Author

rebased against master to pick up commit:f289d9f (which fixed the build for OS-X on master)

@facebook-github-bot
Copy link
Contributor

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gfosco
Copy link
Contributor

gfosco commented Jan 10, 2018

Closing this via automation due to lack of activity. If discussion is still needed here, please re-open or create a new/updated issue.

@gfosco gfosco closed this Jan 10, 2018
@ajkr ajkr reopened this Jan 11, 2018
@ajkr ajkr self-assigned this Jan 11, 2018
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

lgtm. I rebased and fixed conflicts here (ajkr@2919a39) if you want to update this PR with that content.

@stevelittle
Copy link
Author

@ajkr - Your rebased change looks good to me, thanks for doing that. Please go ahead and merge.

@ajkr
Copy link
Contributor

ajkr commented Jun 13, 2018

Thanks, replaced with #3993

@ajkr ajkr closed this Jun 13, 2018
facebook-github-bot pushed a commit that referenced this pull request Jun 14, 2018
Summary:
Rebased and resubmitting #1831 on behalf of stevelittle.

The problem is when a single process attempts to open the same DB twice, the second attempt fails due to LOCK file held. If the second attempt had opened the LOCK file, it'll now need to close it, and closing causes the file to be unlocked. Then, any subsequent attempt to open the DB will succeed, which is the wrong behavior.

The solution was to track which files a process has locked in PosixEnv, and check those before opening a LOCK file.

Fixes #1780.
Closes #3993

Differential Revision: D8398984

Pulled By: ajkr

fbshipit-source-id: 2755fe66950a0c9de63075f932f9e15768041918
rcane pushed a commit to rcane/rocksdb that referenced this pull request Sep 13, 2018
Summary:
Rebased and resubmitting facebook#1831 on behalf of stevelittle.

The problem is when a single process attempts to open the same DB twice, the second attempt fails due to LOCK file held. If the second attempt had opened the LOCK file, it'll now need to close it, and closing causes the file to be unlocked. Then, any subsequent attempt to open the DB will succeed, which is the wrong behavior.

The solution was to track which files a process has locked in PosixEnv, and check those before opening a LOCK file.

Fixes facebook#1780.
Closes facebook#3993

Differential Revision: D8398984

Pulled By: ajkr

fbshipit-source-id: 2755fe66950a0c9de63075f932f9e15768041918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants