Remove locking from handler implementation about tusd

[Acconut]

First of all, I want to thank you for this contribution.
The reason I implemented locking inside the handler is that it shares the same logic across all data store and therefore removing the need of replicating this functionality inside the store. However, my major concern is that your patch may introduce a race condition:

• Request 1 wants to upload data with length 10 starting at offset 0.
• tusd.Handler verifies the offset successfully and passes the data to the store.
• The data store locks the according resource for request 1.
• Request 1 slowly uploads the data but is not finished.
• While request 1 is still uploading, Request 2 wants to upload data with length 20 at offset 0.
• Since request 1 is not finished, the resource's offset is still at 0 and the handler verifies the offset for request 2 successfully and passes the data to the store.
• Since the resource is locked from request 1, request 2 needs to wait for the resource to be unlocked.
• Request 1 finishes and the resource is unlocked.
• The data from request 2 is now written to the resource.

In the end you end up with data at the offset 10, which is meant to be at offset 0. This could be a possible conflict.

I know, that the current solution is not optimal, especially in a system where multiple tusd instances are run, the current locking functionality is useless since locks are bound to one tusd instance.

[adambkaplan]

Thanks for your feedback! Would your scenario arise from a parallel upload situation, which come from the Concatenation extension? I actually haven't looked too closely into that since it's not "official" yet, but clearly I should since this implementation supports it.

I guess it isn't safe to assume that clients will serially upload chunks. Maybe changing the locking map to use sync.Mutex may prove more robust (I can test that out).

[Acconut]

No the Concatenation extension is not involved at all. Basically the issue with your implementation is that the locking happens after the offset has been verified, but it should be before instead.

We considered using sync.Mutex before but we decided against since we don't want to pause one request at all while another one is processing. Imagine, you are waiting for the response 10min just because it's locked. We wanted immediate responses which is not the use case for sync.Mutex.

[adambkaplan]

Good point - the need to have consistent data when verifying the offset is something I need to address in our handler implementation.

[Acconut]

Another option would be to allow the data store to handle locking separated from writing the chunks. This can be achieved by adding a LockFile(id string) and UnlockFile(id string) method to the DataStore interface. The HTTP handler is now able to lock the file before comparing the offset:

store.LockFile(id)
defer store.UnlockFile(id)

// verify offset and proceed…

What do you think?

[adambkaplan]

I've been playing around with a similar idea, and it seems to be working well (added a field called WritePending, validated that before checking offsets...). I like your syntax better - I'll merge it with what I have and will update the pull request sometime next week.

[Acconut]

@abkaplan07 Awesome, looking forward to your updates :)

[adambkaplan]

Updated in the pull request. In addition to the lock/unlock API, I also persisted the locks to the FileInfo objects.

[Acconut]

Your implementation appears to be very robust and promising but there is a single point I am very concerned about: FileStore is not kill-safe. Imagine, one file is locked but before it's unlocked again the tusd instance gets killed before the lock is released again. Now, any FileStore instance will refuse to unlock the file since it does not obtain the lock. Usually you bypass this issue by binding a lock to the process:

• using flock/fcntl system calls (sadly not portable and there doesn't seem to be a package available)
• using separate lock file containing the process' PID (if the PID does not exists anymore, the lock owners has died and we can take the lock; see https://github.com/nightlyone/lockfile)

I will do some detailed research on which solution is the best for our case and then report back :)

[Acconut]

After some research I came to an conclusion (sorry for the delay):

Native locking mechanism using flock/fcntl are not portable by default and event their behaviour differs between various UNIX implementation. Furthermore, not every file system (e.g. older versions of NFS) supports locking. And finally, we had to write our own wrapper around these system calls.

So I would like to take the second approach using a lock in conjunction with the corresponding PID. We only need to decide whether we store this information in the .info file (as your fix does) or in a separate .lock file. Any opinions about this?

[Acconut]

After spending some time on this topic, I decided to go one step further and extract locking from tusd.DataStore into it's own storage proxy. It's pretty much finished as you can see in #37.

[Acconut]

Very similar mechanism has been implemented in #37. Thanks for your efforts and patience :)