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 lock file support #1002

Closed
scic opened this issue Sep 27, 2017 · 11 comments · Fixed by #1231
Closed

Remove lock file support #1002

scic opened this issue Sep 27, 2017 · 11 comments · Fixed by #1231
Assignees
Milestone

Comments

@scic
Copy link

scic commented Sep 27, 2017

KeepassXC creates a lock file when a database was opened. If another user opens the same database he gets a warning that another user has this file opened.
In the meanwhile KeepassXC implemented:

  • Merge support
  • Only one instance of KeepassXC can be opened on the same computer

Thus I think no such lock file is necessary anymore.

Expected Behavior

Multiple users can concurrently open a file on a network share (NFS, SMB, ...) without being shown a dialog where they are informed that other users have opened the file and how to resolve this issue.
The already implemented database merge feature should already be sufficient to handle concurrent edits.

Current Behavior

A dialog is shown with the options:

  • Don't open
  • Open anyways
  • Open read only

Possible Solution

Remove the lock file feature altogether.
I think the feature was introduced 2 years ago with this commit: 05b5446

Context

We share database files over a network share. The database is rarely edited anyways. But read (opened) very often by multiple users concurrently. The dialog just annoys users and it leads to confusion what to do.

Removing this feature would also solve #275

@hifi
Copy link
Member

hifi commented Sep 27, 2017

Indirectly #803 could also be solved by not having a "read only" mode at all but just fail with a dialog if saving doesn't work.

@droidmonkey
Copy link
Member

Im on the fence on this, but you make a compelling case

@louib
Copy link
Member

louib commented Sep 27, 2017

The already implemented database merge feature should already be sufficient to handle concurrent edits.

I'd wait for the merge function to be more mature before relying on it as a synchronisation function for real-time concurrent edits. See #939

@scic
Copy link
Author

scic commented Sep 28, 2017

Even if there would be no merge feature I think creating a lock file is not a good experience.

  • If you only read the database no harm is done.
  • If you write to the database it is sufficient to check before attempting to write if the database changed after it was opened (e.g. last modified date). On save a dialog could even prompt if the new file should be used, discarded or merged. Thus a check before each save operation is sufficient in order not to loose data. There is no need to lock the access for all read operations.

In Short: We only need to guard write operations.

Furthermore the lock file is only understood by KeepassXC. At our place we use multiple Clients like Keepass 2, KeeWeb, MacPass, Keepass2Android, kpcli, etc. Each of these can modify the database anyways and the lock file will not protect KeepassXC users from overwriting changes from other clients. Only doing a check if the database was modified before each write will.

Thus I recommend now: removing lock file support and adding a guard on write. Adding the guard would also fix #771 and help with #90

I just found the description how Keepass does it. I think instead of a lock file we should do it like this: http://keepass.info/help/base/multiuser.html#syncorsave2x

@mkasberg
Copy link
Contributor

I like scic's comment.

I think my use case is pretty common. I keep the file in my Dropbox and the lock file has prevented me from overwriting changes once or twice in the past. A write guard with an option to merge is probably a better user experience.

@droidmonkey
Copy link
Member

This is smart, although instead of using modified times it would be better to save the md5 hash and compare before each save action. Machines on different times can screw up modified times.

@CosmicToast
Copy link

You might also use blake2 hashing - it's faster than md5 and significantly less collision-happy (though with the sizes the db is likely to be, the latter's less significant).

@droidmonkey
Copy link
Member

I'd limit or hash choices to those that qt has implemented for ease of use

@droidmonkey droidmonkey self-assigned this Nov 26, 2017
@droidmonkey droidmonkey added this to the v2.3.0 milestone Nov 26, 2017
@hrlmartins
Copy link

Sorry to reply in such "old" issue but just to be sure of some things:
There were concerns throughout the discussion about not relying entirely on merge function until it is more mature and scic argued that even if no such merge function existed we could rely on checking modified time before writing the file.... but isn't this almost the definition of a concurrency problem?

Let's imagine, the very unlikely case, that two different instances (let's say on a share) open at the same time and save exactly at the same time. There could be the case that both see the file as "not modified since last opened" and try to write to file at the same time which may cause problems. Am I seeing this right or missing something? :P

Thanks and sorry for again for posting in an old issue.

@droidmonkey
Copy link
Member

You are correct, and we discussed in another issue the requirement for a third party server to control locking of the file (transaction based) in that scenario. We don't, and can't, guarantee perfect atomic transactions in all cases. Shared use of the database file is a side effect of network based storage, not an inherent feature in KPXC. Removing the lock file doesn't have much to do with that except for removing the "warning" that the database is also opened elsewhere.

@hrlmartins
Copy link

Yeh, you're right. Thanks!

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

Successfully merging a pull request may close this issue.

7 participants