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

Remove FLOCK or make it optional #988

Closed
ehsannm opened this issue Aug 17, 2019 · 16 comments · Fixed by #1243
Closed

Remove FLOCK or make it optional #988

ehsannm opened this issue Aug 17, 2019 · 16 comments · Fixed by #1243
Labels
help wanted Please help us fix this! kind/enhancement Something could be better. priority/P3 Low priority, something to be done once everything else is fixed. status/accepted We accept to investigate or work on it.

Comments

@ehsannm
Copy link
Contributor

ehsannm commented Aug 17, 2019

Hi,

We are using badger as our data storage in our project. We run badger on Android and iOS devices. We have problem on some androids, which it seems they have problem with the FLOCK. When we ignore the err in Open method [db.go - line: 232] then we have no problem.

dirLockGuard, err = acquireDirectoryLock(opt.Dir, lockFile, opt.ReadOnly)
if err != nil {
return nil, err
}

for the moment we forked, and changed the code to fix our problem. However I was wondering any better solution do you offer ?

@jarifibrahim jarifibrahim added kind/question Something requiring a response priority/P3 Low priority, something to be done once everything else is fixed. status/needs-attention This issue needs more eyes on it, more investigation might be required before accepting/rejecting it labels Aug 17, 2019
@jarifibrahim
Copy link
Contributor

Hey @ehsannm, what's the error you see on android? The directory lock prevents multiple badger instances from accessing the same data directory (unless badger is in read-only mode). Making it optional would be easy but I'd like to understand the problem first.

I do not have much experience with android. Any links or references would be appreciated. :)

@ehsannm
Copy link
Contributor Author

ehsannm commented Aug 19, 2019

Hi @jarifibrahim, Thanks for your response.
Basically this error does not happen on all the androids but a good few of them crash when calling Flock.

avc: denied { lock } for path="/data/data/com.ronaksoftware.river.beta/files/databases/river/badger" dev="vdc" ino=15589 scontext=u:r:untrusted_app:s0:c512,c768 tcontext=u:object_r:app_data_file:s0:c512,c768 tclass=dir permissive=0

It seems that flock in some cases needs root access.

@jarifibrahim
Copy link
Contributor

@ehsannm Could it be possible that the android device has some other app (maybe an anti-virus) which acquire the lock on the same file?
Can you try running on the app on a clean android device and see if the error persist?
Alternatively, could you check which process is acquiring the lock (https://unix.stackexchange.com/questions/85994/how-to-list-processes-locking-file should help) ?

@ehsannm
Copy link
Contributor Author

ehsannm commented Aug 19, 2019

@jarifibrahim It is not happen on all androids, some of them. But our app needs to be compatible with all androids. It may be because of SELinux on some androids. We temporary fixed by changing the code in the badger, but we think if you provide it as an option then we don't need to fork the project for such a little change, or may be another lock mechanism ?!!

@jarifibrahim
Copy link
Contributor

@ehsannm I still feel the issue is with android. The flock system call should've worked for you.

@manishrjain what do you think? Can we make the file locking optional?

@ehsannm
Copy link
Contributor Author

ehsannm commented Aug 19, 2019

@jarifibrahim I think so too, but what could we do? we cannot force users to update their phones, Internally we are making sure that we do not access the badger concurrently because it is in our code but forcing users to update their phone is not practical

@stale
Copy link

stale bot commented Sep 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Sep 18, 2019
@jarifibrahim jarifibrahim added help wanted Please help us fix this! and removed status/stale The issue hasn't had activity for a while and it's marked for closing. labels Sep 18, 2019
@jarifibrahim
Copy link
Contributor

@ehsannm Feel free to send a PR for this.

@ehsannm
Copy link
Contributor Author

ehsannm commented Sep 18, 2019

@jarifibrahim I created a pull request #1046

@stale
Copy link

stale bot commented Oct 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Oct 18, 2019
@ehsannm
Copy link
Contributor Author

ehsannm commented Oct 19, 2019

@jarifibrahim is anything else i need to do ?

@stale stale bot removed the status/stale The issue hasn't had activity for a while and it's marked for closing. label Oct 19, 2019
@stale
Copy link

stale bot commented Nov 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Nov 18, 2019
@stale
Copy link

stale bot commented Nov 25, 2019

This issue was marked as stale and no activity has occurred since then, therefore it will now be closed. Please, reopen if the issue is still relevant.

@jonknight73
Copy link

Are there any updates on this please?

@jarifibrahim jarifibrahim added kind/enhancement Something could be better. status/accepted We accept to investigate or work on it. and removed kind/question Something requiring a response status/needs-attention This issue needs more eyes on it, more investigation might be required before accepting/rejecting it status/stale The issue hasn't had activity for a while and it's marked for closing. labels Mar 5, 2020
@jarifibrahim
Copy link
Contributor

jarifibrahim commented Mar 5, 2020

Hey @jonknight73 @ehsannm the issue got closed because of inactivity. I've reopened it. I've raised a PR to fix this which is based on @ehsannm original PR.

@jonknight73
Copy link

Thank you very much. I'll be delighted to be able to move off my fork back into the main repo.

jarifibrahim pushed a commit that referenced this issue Mar 13, 2020
Fixes #988
Based on #1046

This PR adds `BypassDirLock` option which allows badger to work without
acquiring a lock on the data directory. This option could lead to data
corruption if used with multiple badger instances trying to write to
the same badger directory.

Co-authored-by: Ehsan Noureddin Moosa <[email protected]>
jarifibrahim pushed a commit that referenced this issue Mar 13, 2020
Fixes #988
Based on #1046

This PR adds `BypassDirLock` option which allows badger to work without
acquiring a lock on the data directory. This option could lead to data
corruption if used with multiple badger instances trying to write to
the same badger directory.

Co-authored-by: Ehsan Noureddin Moosa <[email protected]>
(cherry picked from commit 1bcbefc)
jarifibrahim pushed a commit that referenced this issue Mar 24, 2020
Fixes #988
Based on #1046

This PR adds `BypassDirLock` option which allows badger to work without
acquiring a lock on the data directory. This option could lead to data
corruption if used with multiple badger instances trying to write to
the same badger directory.

Co-authored-by: Ehsan Noureddin Moosa <[email protected]>
(cherry picked from commit 1bcbefc)
manishrjain pushed a commit to outcaste-io/outserv that referenced this issue Jul 6, 2022
Fixes hypermodeinc/badger#988
Based on hypermodeinc/badger#1046

This PR adds `BypassDirLock` option which allows badger to work without
acquiring a lock on the data directory. This option could lead to data
corruption if used with multiple badger instances trying to write to
the same badger directory.

Co-authored-by: Ehsan Noureddin Moosa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Please help us fix this! kind/enhancement Something could be better. priority/P3 Low priority, something to be done once everything else is fixed. status/accepted We accept to investigate or work on it.
Development

Successfully merging a pull request may close this issue.

3 participants