Only allow running a single pick instance concurrently #171

leonklingele · 2018-03-17T14:34:50Z

To prevent data loss, only a single pick instance should be running
at the same time.
Without this patch, if an active "pick note" editor is open and
new credentials are stored in a pick safe, closing the "pick note"
editor will overwrite all changes happened since.

leonklingele · 2018-03-17T14:37:05Z

backends/s3/client.go

@@ -75,6 +76,16 @@ func (c *client) Backup() error {
 	return c.putObject(bytes.NewReader(data), c.Bucket, backupKey)
 }

+func (c *client) Lock() error {
+	// TODO: Implement me!


@bndw maybe you find the time to implement this? Please merge this PR asap and fix in a separate PR :)

bndw · 2018-03-19T18:50:50Z

@leonklingele I think in order to implement locking correctly we'll need to handle a few cases. What happens if a lock is somehow orphaned? We may consider TTLs.

leonklingele · 2018-03-19T19:00:03Z

@bndw the lock is automatically released once pick terminates. Try it yourself :)

bndw · 2018-03-20T02:44:45Z

@leonklingele My only concern is the case where the binary may not terminate, for example running pick on a remote server. In that case I believe the safe could become "permanently" locked.

leonklingele · 2018-03-20T08:04:31Z

Updated to only fail if at least two instances of pick try to open the safe in writable mode.

for example running pick on a remote server

What do you mean by "running pick on a remote server"? pick can only be run locally?!
If you intended to say "running pick with a remote safe" (e.g. using the S3 backend), well, then the S3 backend needs to implement some proper way of ensuring the lock really is released once the pick binary terminates.
The file backend as implemented in this PR takes care that the lock doesn't become stale.

In that case I believe the safe could become "permanently" locked

I still don't see how that could work, please give some more insight to your thoughts :)

bndw · 2018-03-20T17:21:52Z

@leonklingele I was imagining running pick on a remote server that's accessed via SSH;

SSH to remote server
open a note for editing
connection fails

In this [edge] case, is it possible that pick may not exit and "permanently" lock the safe?

leonklingele · 2018-03-20T18:37:42Z

Well, if your SSH connection breaks for whatever reason while pick is running (e.g. a pick note command), then yes, the pick binary might not terminate and you need to killall it manually.
The exact same issue also occurs when you're in the process of apt upgrade'ing your system and the SSH connection breaks, so I don't really see this as a problem only we face. Just killall pick and you're good to go — or consider running pick inside a tmux or screen session as it's recommended to do with apt.
Do you have a better solution? Your described edge-case is a minor problem compared to losing credentials caused by running multiple pick instances (which happened to me in the past.. sucks)

bndw · 2018-03-20T22:03:13Z

I agree your use-case is much more likely and significant, however I just want to ensure I understand the problem space thoroughly.

I'll review this week and get it released.

bndw · 2018-04-02T03:18:25Z

backends/s3/client.go

+}
+
+func (c *client) Lock() error {
+	// TODO: Implement me!


I need to come up with a solution for s3 before merging this, otherwise it'll break for me.

It shouldn't break, starting pick only fails if a backend's Lock() method returns errors.ErrAlreadyRunning, see https://github.com/bndw/pick/pull/171/files#diff-c07200a8f18f2a355ec0bd360f843b40R61
Both s3/client::Lock() and s3/client::Unlock() return a different error.

.. not to say it shouldn't be implemented asap —s3 support is still dangerous to use.

Agreed, tracking in #174

To prevent data loss, only a single pick instance should be running at the same time. Without this patch, if an active "pick note" editor is open and new credentials are stored in a pick safe, closing the "pick note" editor will overwrite all changes happened since.

leonklingele · 2018-04-02T10:19:06Z

commands/note.go

 	if err != nil {
 		return err
 	}

+	if action == "edit" {


This moved down a few lines to avoid printing the message in case the safe loader fails to do its job.

leonklingele · 2018-04-02T10:23:54Z

backends/s3/client.go

+}
+
+func (c *client) Lock() error {
+	// TODO: Implement me!


It shouldn't break, starting pick only fails if a backend's Lock() method returns errors.ErrAlreadyRunning, see https://github.com/bndw/pick/pull/171/files#diff-c07200a8f18f2a355ec0bd360f843b40R61
Both s3/client::Lock() and s3/client::Unlock() return a different error.

leonklingele · 2018-04-02T10:24:56Z

Rebased to latest develop.

leonklingele · 2018-04-18T23:02:35Z

This should be published to master asap, what about an emergency release?

bndw · 2018-04-19T00:07:19Z

@leonklingele I'm currently trying to implement locking for s3 backend before doing a master push/release--

I'm a little confused on the SetWritable and Unlock behavior. I understand that the lock for the file backend is automatically released, however for other backends we need an explicit call to Unlock and the existing implementation doesn't appear to have that hook in there.

In my mind, any edit operation (e.g. add, rm, etc) should call

backend.Lock()
defer backend.Unlock()

Do you have any advice on the current implementation with regard to this?

leonklingele · 2018-04-19T00:57:17Z

I'm a little confused on the SetWritable and Unlock behavior.

You mean Lock and Unlock, right?

The lock for the file backend is automatically released by the kernel once pick terminates, regardless of the signal it received (SIGINT, SIGKILL, etc.). This ensures the lock will eventually be unlocked again and under no circumstance be held for longer than required. The locking happens on an atomic level (i.e. there will be no lock-acquiring data race between two instances of the app).

For the S3 backend it will probably get a bit trickier though. Not only do we need to ensure that only a single lock can exist, we also need to ensure it will get removed when no longer required (even if pick is kill -9'ed). Maybe there's some locking mechanism supported by Amazon? I don't know how up-to-date this answer is, but it might help anyway: https://stackoverflow.com/questions/3431418/locking-with-s3/3434952#3434952

Another (good?) approach to the problem would be to rely on a proper safe-syncing mechanism (instead of locking).. Just an idea which came to my mind, I haven't really thought this through yet.

bndw · 2018-04-20T00:23:47Z

I agree, cleaning up locks will be tricky given the OS signals. Maybe the best way forward for the time being is to not support, or no-op, locking in the S3 backend. We can take this on as tech debt until we figure out a solution.

leonklingele · 2018-04-24T19:20:33Z

bump

bndw · 2018-04-25T21:42:43Z

@leonklingele https://github.com/bndw/pick/releases/tag/v0.7.0

leonklingele commented Mar 17, 2018

View reviewed changes

leonklingele force-pushed the single-instance branch from f3d8740 to 526463d Compare March 20, 2018 07:52

leonklingele force-pushed the single-instance branch 3 times, most recently from 7b41995 to 4af7f25 Compare March 20, 2018 11:08

bndw reviewed Apr 2, 2018

View reviewed changes

leonklingele force-pushed the single-instance branch from 4af7f25 to ec8521d Compare April 2, 2018 10:12

leonklingele force-pushed the single-instance branch from ec8521d to e501a72 Compare April 2, 2018 10:14

leonklingele commented Apr 2, 2018

View reviewed changes

bndw merged commit f0e539d into bndw:develop Apr 6, 2018

bndw mentioned this pull request Apr 6, 2018

Implement locking in s3 backend #174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow running a single pick instance concurrently #171

Only allow running a single pick instance concurrently #171

leonklingele commented Mar 17, 2018

leonklingele Mar 17, 2018

bndw commented Mar 19, 2018

leonklingele commented Mar 19, 2018

bndw commented Mar 20, 2018

leonklingele commented Mar 20, 2018

bndw commented Mar 20, 2018

leonklingele commented Mar 20, 2018

bndw commented Mar 20, 2018

bndw Apr 2, 2018

leonklingele Apr 2, 2018

leonklingele Apr 6, 2018

bndw Apr 6, 2018

leonklingele Apr 2, 2018

leonklingele Apr 2, 2018

leonklingele commented Apr 2, 2018

leonklingele commented Apr 18, 2018

bndw commented Apr 19, 2018

leonklingele commented Apr 19, 2018

bndw commented Apr 20, 2018

leonklingele commented Apr 24, 2018

bndw commented Apr 25, 2018

Only allow running a single pick instance concurrently #171

Only allow running a single pick instance concurrently #171

Conversation

leonklingele commented Mar 17, 2018

leonklingele Mar 17, 2018

Choose a reason for hiding this comment

bndw commented Mar 19, 2018

leonklingele commented Mar 19, 2018

bndw commented Mar 20, 2018

leonklingele commented Mar 20, 2018

bndw commented Mar 20, 2018

leonklingele commented Mar 20, 2018

bndw commented Mar 20, 2018

bndw Apr 2, 2018

Choose a reason for hiding this comment

leonklingele Apr 2, 2018

Choose a reason for hiding this comment

leonklingele Apr 6, 2018

Choose a reason for hiding this comment

bndw Apr 6, 2018

Choose a reason for hiding this comment

leonklingele Apr 2, 2018

Choose a reason for hiding this comment

leonklingele Apr 2, 2018

Choose a reason for hiding this comment

leonklingele commented Apr 2, 2018

leonklingele commented Apr 18, 2018

bndw commented Apr 19, 2018

leonklingele commented Apr 19, 2018

bndw commented Apr 20, 2018

leonklingele commented Apr 24, 2018

bndw commented Apr 25, 2018