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

Clients should lock .tox file #28

Open
benwaffle opened this issue Feb 7, 2016 · 18 comments
Open

Clients should lock .tox file #28

benwaffle opened this issue Feb 7, 2016 · 18 comments

Comments

@benwaffle
Copy link

Clients should lock the .tox file so that you can't run multiple tox clients on the same profile. Things get buggy if you do that, and I'm guessing you also have the possibility of losing your data.

https://en.wikipedia.org/wiki/File_locking

@benwaffle benwaffle changed the title Lock file Clients should lock .tox file Feb 7, 2016
@nurupo
Copy link
Member

nurupo commented Feb 7, 2016

.tox files are not required, so requiring them to be locked doesn't make sense.

@nurupo
Copy link
Member

nurupo commented Feb 7, 2016

@benwaffle
Copy link
Author

If a client uses a .tox file in the standard location (e.g. ~/.config/tox/), it should lock it until the program ends

@nurupo
Copy link
Member

nurupo commented Feb 7, 2016

Ok, now this sounds reasonable.

@benwaffle
Copy link
Author

Also, clients should respect locked .tox files (on linux, file locking is advisory, not mandatory)

@GrayHatter
Copy link

no I don't think so... 👎

@benwaffle
Copy link
Author

So we need to avoid the issue of Windows programs crashing and leaving files locked and unusable by any program until a reboot.

Also, locks should also be advisory and not mandatory because the .tox files need to be read for e.g. displaying a profile chooser

@SkyzohKey
Copy link

👍

@Zer0-One
Copy link
Member

Zer0-One commented Mar 8, 2016

This sounds reasonable to me.

@JFreegman thoughts?

@GrayHatter
Copy link

As discussed in IRC windows doesn't really have an "advisory lock" solution.

And again, as discussed on IRC, renaming the file would be the best/easiest/crossplatform solution.

@Chuongv
Copy link
Member

Chuongv commented Mar 8, 2016

Sounds like a good idea to me. Would like to see the PR for this.

@SkyzohKey
Copy link

@GrayHatter How renaming the file would save the issue ? You'll be at a point obliged to rename it back to the original name, and it can produce writing race.

@GrayHatter
Copy link

@SkyzohKey what would the race condition be?

@SkyzohKey
Copy link

@GrayHatter If you rename the profil before using it without locking it, others clients would still be able to load it and maybe produce issues.

@GrayHatter
Copy link

if you rename it to profile.tox.inuse, and another client tries to write to that. The other client would be broken... that's like having a race condition with rm -rf

@SkyzohKey
Copy link

SkyzohKey commented Apr 27, 2016

@GrayHatter Nah, when the user close it's client, the profile can get "raced" at final rename stage.
Example:

  • User load test.tox with qTox
    • qTox rename the file to test.tox.inuse
  • User do chatting
  • User load test.tox with Ricin
    1. Ricin rename the file to test.tox.inuse
      • Race.
    2. or Ricin rename the file to `test.tox.inuse{random(0,8)}
  • User close qTox
    • qTox rename the file back to test.tox
  • Ricin still use test.tox.inuse{random(0,8)}
  • User close Ricin
    • Ricin rename the file to test.tox
      • Changes in the profile saved by qTox are overrided by Ricin

EDIT: The simplest way would be creating a test.tox.lock file that would indicate to others client that they musn't load this profile since it is already in use. The .lock file would only contains the PID of the client using the profile currently.

@GrayHatter
Copy link

I stand by what I said before... one of those clients is broken.

profile.tox -> start qTox -> rename profile.tox to profile.tox.inuse

Start ricin -> it sees profile.tox.inuse -> dispalys error, unable to start profile.tox, profile.tox.inuse this may happen if ricin was force closed/crashed or there may be another tox client using this profile, . Would you like to attempt to recover profile.tox? -> user clicks yes -> please make sure no other client using this profile -> user clicks yes

No race condition unless the user is stupid, and you can't open a PR for stupid.

text.tox.lock with the pid is also a good idea, would allow for checking if that process is still running, if so, exit helpfully (or start a new profile).

@nurupo
Copy link
Member

nurupo commented Apr 27, 2016

I don't get why you are talking about different clients and about tox files. Clients are free to store the profile in any format, even in a database, and they can do so in any place -- clients don't have to store profile as ~/.config/tox/data.tox file. As I see it, clients should store the profiles in their client-specific subdirs or in some other way, to prevent issues of sharing profiles with other clients. The whole idea of sharing files between clients is bad, as you need to standardize everything and strip clients of freedom of choosing formats to store the data in and paths to store the data at. It's a lot better to have a defined export/import formats which clients can implement, which is already specified by TCS. So if a user would like to use a profile from one client in another client, the clients should provide export/import functionality, that would export the profile using TCS specified format and import it from such format. You don't need to care of other client using your profile files accidentally (if they do access files from your client's subdir then it's not client's fault and there is nothing you can do about it), all you need to take care of is to prevent multiple instances of the same client from using the same profile at the same time, which doesn't have to be implemented as file locking, any working implementation should suffice, as far as the standard is concerned. So, this issue should really say: "A client shouldn't allow the user to run more than one instances of the same profile".

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

No branches or pull requests

6 participants