Comments (12)
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.
from tusd.
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).
from tusd.
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.
from tusd.
Good point - the need to have consistent data when verifying the offset is something I need to address in our handler implementation.
from tusd.
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?
from tusd.
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.
from tusd.
@abkaplan07 Awesome, looking forward to your updates :)
from tusd.
Updated in the pull request. In addition to the lock/unlock API, I also persisted the locks to the FileInfo objects.
from tusd.
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 :)
from tusd.
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?
from tusd.
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.
from tusd.
Very similar mechanism has been implemented in #37. Thanks for your efforts and patience :)
from tusd.
Related Issues (20)
- pre-finish blocking script problem HOT 1
- validating video files using blocking hooks HOT 4
- Support for managed identity for Azure blob storage HOT 1
- AzureService is using a deprecated SDK (github.com/Azure/azure-storage-blob-go/azblob) HOT 1
- hooks: Hooks continue failing after plugin crashes
- Getting error about part sizes with Cloudflare R2 bucket HOT 3
- Up to date Golang client? HOT 3
- While passing s3Client to s3store.New getting an error named missing method AbortMultipartUploadWithContext HOT 2
- Return 4xx errors from Hooks as 4xx errors. HOT 3
- Best way to pass a server variable to the JS client HOT 3
- How to delete files uploaded to server? HOT 1
- Duplicate id printing in log HOT 1
- `InternalServerError: no such file or directory` when Downloading file with using File Locker. HOT 1
- Extract ID from Path is broken if base path is provided HOT 6
- s3store: unable to create multipart upload HOT 1
- Example haproxy config HOT 11
- Local storage - folder hierarchy HOT 4
- Checking current usage vs limit using /quota endpoint. HOT 3
- FileInfoChanges set in PreFinishHook not available in PostFinishHook HOT 2
- Is lockUpload really needed for HEAD? HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from tusd.