From 49d7c2ff784345a9d65ae2f50173e74c6333a2e5 Mon Sep 17 00:00:00 2001 From: Marius Date: Wed, 9 Dec 2015 20:25:08 +0100 Subject: [PATCH 01/17] Separate locking into data store implementation The goal is to allow different locking mechanism to be used for different storages. For example, in-memory for very few uploads, a file locker in conjunction with the FileStore or an external service, such as ZooKeeper. --- lockingstore/lockingstore.go | 67 ++++++++++++++++++++++++++++++++++++ lockingstore/memorylocker.go | 35 +++++++++++++++++++ unrouted_handler.go | 51 ++------------------------- 3 files changed, 105 insertions(+), 48 deletions(-) create mode 100644 lockingstore/lockingstore.go create mode 100644 lockingstore/memorylocker.go diff --git a/lockingstore/lockingstore.go b/lockingstore/lockingstore.go new file mode 100644 index 0000000..6e5b1fa --- /dev/null +++ b/lockingstore/lockingstore.go @@ -0,0 +1,67 @@ +package lockingstore + +type Locker interface { + LockUpload(id string) error + UnlockUpload(id string) error +} + +type LockingStore struct { + tusd.DataStore + Locker *Locker +} + +func (store LockingStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { + if err := store.LockUpload(id); err != nil { + return 0, err + } + + defer func() { + if unlockErr := store.UnlockUpload(id); unlockErr != nil { + err = unlockErr + } + }() + + return store.DataStore.WriteChunk(id, offset, src) +} + +func (store LockingStore) GetInfo(id string) (FileInfo, error) { + if err := store.LockUpload(id); err != nil { + return nil, err + } + + defer func() { + if unlockErr := store.UnlockUpload(id); unlockErr != nil { + err = unlockErr + } + }() + + return store.DataStore.GetInfo(id) +} + +func (store LockingStore) GetReader(id string) (io.Reader, error) { + if err := store.LockUpload(id); err != nil { + return nil, err + } + + defer func() { + if unlockErr := store.UnlockUpload(id); unlockErr != nil { + err = unlockErr + } + }() + + return store.DataStore.GetReader(id) +} + +func (store LockingStore) Terminate(id string) error { + if err := store.LockUpload(id); err != nil { + return err + } + + defer func() { + if unlockErr := store.UnlockUpload(id); unlockErr != nil { + err = unlockErr + } + }() + + return store.DataStore.Terminate(id) +} diff --git a/lockingstore/memorylocker.go b/lockingstore/memorylocker.go new file mode 100644 index 0000000..1dd6d22 --- /dev/null +++ b/lockingstore/memorylocker.go @@ -0,0 +1,35 @@ +package lockingstore + +import ( + "github.com/tus/tusd" +) + +type MemoryLocker struct { + locks map[string]bool +} + +func New() *MemoryLocker { + return &MemoryLocker{ + locks: make(map[string]bool), + } +} + +func (locker *MemoryLocker) LockUpload(id string) error { + + // Ensure file is not locked + if _, ok := locker.locks[id]; ok { + return tusd.ErrFileLocked + } + + handler.locks[id] = true + + return nil +} + +func (locker *MemoryLocker) UnlockUpload(id string) error { + // Deleting a non-existing key does not end in unexpected errors or panic + // since this operation results in a no-op + delete(locker.locks, id) + + return nil +} diff --git a/unrouted_handler.go b/unrouted_handler.go index a980752..a17249b 100644 --- a/unrouted_handler.go +++ b/unrouted_handler.go @@ -75,7 +75,6 @@ type UnroutedHandler struct { dataStore DataStore isBasePathAbs bool basePath string - locks map[string]bool logger *log.Logger // For each finished upload the corresponding info object will be sent using @@ -114,7 +113,6 @@ func NewUnroutedHandler(config Config) (*UnroutedHandler, error) { dataStore: config.DataStore, basePath: base, isBasePathAbs: uri.IsAbs(), - locks: make(map[string]bool), CompleteUploads: make(chan FileInfo), logger: logger, } @@ -288,17 +286,16 @@ func (handler *UnroutedHandler) HeadFile(w http.ResponseWriter, r *http.Request) w.WriteHeader(http.StatusNoContent) } -// PatchFile adds a chunk to an upload. Only allowed if the upload is not -// locked and enough space is left. +// PatchFile adds a chunk to an upload. Only allowed enough space is left. func (handler *UnroutedHandler) PatchFile(w http.ResponseWriter, r *http.Request) { - //Check for presence of application/offset+octet-stream + // Check for presence of application/offset+octet-stream if r.Header.Get("Content-Type") != "application/offset+octet-stream" { handler.sendError(w, r, ErrInvalidContentType) return } - //Check for presence of a valid Upload-Offset Header + // Check for presence of a valid Upload-Offset Header offset, err := strconv.ParseInt(r.Header.Get("Upload-Offset"), 10, 64) if err != nil || offset < 0 { handler.sendError(w, r, ErrInvalidOffset) @@ -311,20 +308,6 @@ func (handler *UnroutedHandler) PatchFile(w http.ResponseWriter, r *http.Request return } - // Ensure file is not locked - if _, ok := handler.locks[id]; ok { - handler.sendError(w, r, ErrFileLocked) - return - } - - // Lock file for further writes (heads are allowed) - handler.locks[id] = true - - // File will be unlocked regardless of an error or success - defer func() { - delete(handler.locks, id) - }() - info, err := handler.dataStore.GetInfo(id) if err != nil { handler.sendError(w, r, err) @@ -387,20 +370,6 @@ func (handler *UnroutedHandler) GetFile(w http.ResponseWriter, r *http.Request) return } - // Ensure file is not locked - if _, ok := handler.locks[id]; ok { - handler.sendError(w, r, ErrFileLocked) - return - } - - // Lock file for further writes (heads are allowed) - handler.locks[id] = true - - // File will be unlocked regardless of an error or success - defer func() { - delete(handler.locks, id) - }() - info, err := handler.dataStore.GetInfo(id) if err != nil { handler.sendError(w, r, err) @@ -438,20 +407,6 @@ func (handler *UnroutedHandler) DelFile(w http.ResponseWriter, r *http.Request) return } - // Ensure file is not locked - if _, ok := handler.locks[id]; ok { - handler.sendError(w, r, ErrFileLocked) - return - } - - // Lock file for further writes (heads are allowed) - handler.locks[id] = true - - // File will be unlocked regardless of an error or success - defer func() { - delete(handler.locks, id) - }() - err = handler.dataStore.Terminate(id) if err != nil { handler.sendError(w, r, err) From 45936806a968b30b6de37e5e86fb8770cf170f25 Mon Sep 17 00:00:00 2001 From: Marius Date: Wed, 9 Dec 2015 20:48:41 +0100 Subject: [PATCH 02/17] Add test for lockingstore.MemoryLocker --- lockingstore/lockingstore.go | 34 ++++++++++++++++++------------- lockingstore/memorylocker.go | 2 +- lockingstore/memorylocker_test.go | 27 ++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 15 deletions(-) create mode 100644 lockingstore/memorylocker_test.go diff --git a/lockingstore/lockingstore.go b/lockingstore/lockingstore.go index 6e5b1fa..ca59c89 100644 --- a/lockingstore/lockingstore.go +++ b/lockingstore/lockingstore.go @@ -1,5 +1,11 @@ package lockingstore +import ( + "io" + + "github.com/tus/tusd" +) + type Locker interface { LockUpload(id string) error UnlockUpload(id string) error @@ -7,16 +13,16 @@ type Locker interface { type LockingStore struct { tusd.DataStore - Locker *Locker + Locker Locker } -func (store LockingStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { - if err := store.LockUpload(id); err != nil { +func (store LockingStore) WriteChunk(id string, offset int64, src io.Reader) (n int64, err error) { + if err := store.Locker.LockUpload(id); err != nil { return 0, err } defer func() { - if unlockErr := store.UnlockUpload(id); unlockErr != nil { + if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { err = unlockErr } }() @@ -24,13 +30,13 @@ func (store LockingStore) WriteChunk(id string, offset int64, src io.Reader) (in return store.DataStore.WriteChunk(id, offset, src) } -func (store LockingStore) GetInfo(id string) (FileInfo, error) { - if err := store.LockUpload(id); err != nil { - return nil, err +func (store LockingStore) GetInfo(id string) (info tusd.FileInfo, err error) { + if err := store.Locker.LockUpload(id); err != nil { + return info, err } defer func() { - if unlockErr := store.UnlockUpload(id); unlockErr != nil { + if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { err = unlockErr } }() @@ -38,13 +44,13 @@ func (store LockingStore) GetInfo(id string) (FileInfo, error) { return store.DataStore.GetInfo(id) } -func (store LockingStore) GetReader(id string) (io.Reader, error) { - if err := store.LockUpload(id); err != nil { +func (store LockingStore) GetReader(id string) (src io.Reader, err error) { + if err := store.Locker.LockUpload(id); err != nil { return nil, err } defer func() { - if unlockErr := store.UnlockUpload(id); unlockErr != nil { + if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { err = unlockErr } }() @@ -52,13 +58,13 @@ func (store LockingStore) GetReader(id string) (io.Reader, error) { return store.DataStore.GetReader(id) } -func (store LockingStore) Terminate(id string) error { - if err := store.LockUpload(id); err != nil { +func (store LockingStore) Terminate(id string) (err error) { + if err := store.Locker.LockUpload(id); err != nil { return err } defer func() { - if unlockErr := store.UnlockUpload(id); unlockErr != nil { + if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { err = unlockErr } }() diff --git a/lockingstore/memorylocker.go b/lockingstore/memorylocker.go index 1dd6d22..b8722ec 100644 --- a/lockingstore/memorylocker.go +++ b/lockingstore/memorylocker.go @@ -21,7 +21,7 @@ func (locker *MemoryLocker) LockUpload(id string) error { return tusd.ErrFileLocked } - handler.locks[id] = true + locker.locks[id] = true return nil } diff --git a/lockingstore/memorylocker_test.go b/lockingstore/memorylocker_test.go new file mode 100644 index 0000000..01ca2c6 --- /dev/null +++ b/lockingstore/memorylocker_test.go @@ -0,0 +1,27 @@ +package lockingstore + +import ( + "testing" + + "github.com/tus/tusd" +) + +func TestMemoryLocker(t *testing.T) { + locker := New() + + if err := locker.LockUpload("one"); err != nil { + t.Errorf("unexpected error when locking file: %s", err) + } + + if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { + t.Errorf("expected error when locking locked file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file again: %s", err) + } +} From b3fb3a3f5da6749beb54a2e1b1d5e19f8ce70a5c Mon Sep 17 00:00:00 2001 From: Marius Date: Tue, 15 Dec 2015 22:04:12 +0100 Subject: [PATCH 03/17] Ensure MemoryLocker implements Locker in tests --- lockingstore/memorylocker_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lockingstore/memorylocker_test.go b/lockingstore/memorylocker_test.go index 01ca2c6..d1b16e2 100644 --- a/lockingstore/memorylocker_test.go +++ b/lockingstore/memorylocker_test.go @@ -7,7 +7,8 @@ import ( ) func TestMemoryLocker(t *testing.T) { - locker := New() + var locker Locker + locker = New() if err := locker.LockUpload("one"); err != nil { t.Errorf("unexpected error when locking file: %s", err) From 4073f4ae64237bd9c835e64ac4695f707208dfd8 Mon Sep 17 00:00:00 2001 From: Marius Date: Tue, 15 Dec 2015 22:59:58 +0100 Subject: [PATCH 04/17] Add test for LockingStore --- lockingstore/lockingstore_test.go | 82 +++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 lockingstore/lockingstore_test.go diff --git a/lockingstore/lockingstore_test.go b/lockingstore/lockingstore_test.go new file mode 100644 index 0000000..4fd3abc --- /dev/null +++ b/lockingstore/lockingstore_test.go @@ -0,0 +1,82 @@ +package lockingstore_test + +import ( + "io" + "testing" + + "github.com/tus/tusd" + . "github.com/tus/tusd/lockingstore" +) + +type store struct { + calls int +} + +func (store *store) NewUpload(info tusd.FileInfo) (string, error) { + return "", nil +} +func (store *store) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { + store.calls += 1 + return 0, nil +} + +func (store *store) GetInfo(id string) (tusd.FileInfo, error) { + store.calls += 1 + return tusd.FileInfo{}, nil +} + +func (store *store) GetReader(id string) (io.Reader, error) { + store.calls += 1 + return nil, nil +} + +func (store *store) Terminate(id string) error { + store.calls += 1 + return nil +} + +type locker struct { + lockCalls int + unlockCalls int +} + +func (locker *locker) LockUpload(id string) error { + locker.lockCalls += 1 + if id == "no" { + return tusd.ErrFileLocked + } + return nil +} + +func (locker *locker) UnlockUpload(id string) error { + locker.unlockCalls += 1 + return nil +} + +func TestLockingStore(t *testing.T) { + locker := new(locker) + store := new(store) + lstore := LockingStore{ + DataStore: store, + Locker: locker, + } + + lstore.NewUpload(tusd.FileInfo{}) + lstore.WriteChunk("", 0, nil) + lstore.GetInfo("") + lstore.GetReader("") + lstore.Terminate("") + + lstore.WriteChunk("no", 0, nil) + lstore.GetInfo("no") + lstore.GetReader("no") + lstore.Terminate("no") + + if locker.lockCalls != 8 { + t.Error("expected 8 calls to LockUpload, but got %d", locker.lockCalls) + } + + if locker.unlockCalls != 4 { + t.Error("expected 8 calls to UnlockUpload, but got %d", locker.unlockCalls) + } +} From b03ddd1d4c2f10ce067ad5866da4e3dc9a83f869 Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 18 Dec 2015 23:13:00 +0100 Subject: [PATCH 05/17] Add documentation for package lockingstore --- lockingstore/lockingstore.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/lockingstore/lockingstore.go b/lockingstore/lockingstore.go index ca59c89..13019af 100644 --- a/lockingstore/lockingstore.go +++ b/lockingstore/lockingstore.go @@ -1,3 +1,12 @@ +// Package lockingstore manages concurrent access to a single upload. +// +// When multiple processes are attempting to access an upload, whether it be +// by reading or writing, a syncronization mechanism is required to prevent +// data corruption, especially to ensure correct offset values and the proper +// order of chunks inside a single upload. +// +// This package wrappes an existing data storage and only allows a single access +// at a time by using an exclusive locking mechanism. package lockingstore import ( @@ -6,13 +15,30 @@ import ( "github.com/tus/tusd" ) +// Locker is the interface required for custom lock persisting mechanisms. +// Common ways to store this information is in memory, on disk or using an +// external service, such as ZooKeeper. type Locker interface { + // LockUpload attempts to obtain an exclusive lock for the upload specified + // by its id. + // If this operation fails because the resource is already locked, the + // tusd.ErrFileLocked must be returned. If no error is returned, the attempt + // is consider to be successful and the upload to be locked until UnlockUpload + // is invoked for the same upload. LockUpload(id string) error + // UnlockUpload releases an existing lock for the given upload. UnlockUpload(id string) error } +// LockingStore wraps an existing data storage and catches all operation. +// Before passing the method calls to the underlying backend, locks are required +// to be obtained. type LockingStore struct { + // The underlying data storage to which the operation will be passed if an + // upload is not locked. tusd.DataStore + // The custom locking persisting mechanism used for obtaining and releasing + // locks. Locker Locker } From 47492260e8821901b5a778ce34f29812bffa912e Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 18 Dec 2015 23:20:52 +0100 Subject: [PATCH 06/17] Add documentation for MemoryLocker --- lockingstore/memorylocker.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lockingstore/memorylocker.go b/lockingstore/memorylocker.go index b8722ec..6536eb9 100644 --- a/lockingstore/memorylocker.go +++ b/lockingstore/memorylocker.go @@ -4,16 +4,21 @@ import ( "github.com/tus/tusd" ) +// MemoryLocker persists locks using memory and therefore allowing a simple and +// cheap mechansim. Locks will only exist as long as this object is kept in +// reference and will be erased if the program exits. type MemoryLocker struct { locks map[string]bool } +// New creates a new lock memory persistor. func New() *MemoryLocker { return &MemoryLocker{ locks: make(map[string]bool), } } +// LockUpload tries to obtain the exclusive lock. func (locker *MemoryLocker) LockUpload(id string) error { // Ensure file is not locked @@ -26,6 +31,7 @@ func (locker *MemoryLocker) LockUpload(id string) error { return nil } +// UnlockUpload releases a lock. If no such lock exists, no error will be returned. func (locker *MemoryLocker) UnlockUpload(id string) error { // Deleting a non-existing key does not end in unexpected errors or panic // since this operation results in a no-op From 97f2efb6e536dacc8be4da9f9585a1c1ceaabbaf Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 18 Dec 2015 23:21:13 +0100 Subject: [PATCH 07/17] Rename New to NewMemoryLocker --- lockingstore/memorylocker.go | 2 +- lockingstore/memorylocker_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lockingstore/memorylocker.go b/lockingstore/memorylocker.go index 6536eb9..d15ecc0 100644 --- a/lockingstore/memorylocker.go +++ b/lockingstore/memorylocker.go @@ -12,7 +12,7 @@ type MemoryLocker struct { } // New creates a new lock memory persistor. -func New() *MemoryLocker { +func NewMemoryLocker() *MemoryLocker { return &MemoryLocker{ locks: make(map[string]bool), } diff --git a/lockingstore/memorylocker_test.go b/lockingstore/memorylocker_test.go index d1b16e2..d14fc7b 100644 --- a/lockingstore/memorylocker_test.go +++ b/lockingstore/memorylocker_test.go @@ -8,7 +8,7 @@ import ( func TestMemoryLocker(t *testing.T) { var locker Locker - locker = New() + locker = NewMemoryLocker() if err := locker.LockUpload("one"); err != nil { t.Errorf("unexpected error when locking file: %s", err) From 8d94d5532074396d708547ba6755aa29aa0d6f10 Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 18 Dec 2015 23:24:12 +0100 Subject: [PATCH 08/17] Add package descriptions for documentation --- filestore/filestore.go | 2 ++ handler.go | 1 + limitedstore/limitedstore.go | 4 +++- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/filestore/filestore.go b/filestore/filestore.go index a3a80b6..e524724 100644 --- a/filestore/filestore.go +++ b/filestore/filestore.go @@ -1,3 +1,5 @@ +// Package filestore provide a storage backend based on the local file system. +// // FileStore is a storage backend used as a tusd.DataStore in tusd.NewHandler. // It stores the uploads in a directory specified in two different files: The // `[id].info` files are used to store the fileinfo in JSON format. The diff --git a/handler.go b/handler.go index 2a8a9ba..aafb1e2 100644 --- a/handler.go +++ b/handler.go @@ -1,3 +1,4 @@ +// Package tusd provides ways to accept tusd calls using HTTP. package tusd import ( diff --git a/limitedstore/limitedstore.go b/limitedstore/limitedstore.go index c77ad5f..7b6da8c 100644 --- a/limitedstore/limitedstore.go +++ b/limitedstore/limitedstore.go @@ -1,4 +1,6 @@ -// Package limitedstore implements a simple wrapper around existing +// Package limitedstore provides a storage with a limited space. +// +// This goal is achieved by using a simple wrapper around existing // datastores (tusd.DataStore) while limiting the used storage size. // It will start terminating existing uploads if not enough space is left in // order to create a new upload. From 83587ca0f85c912dda60f5e08a0579b9353254db Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 19 Dec 2015 00:02:11 +0100 Subject: [PATCH 09/17] Add filestore.FileLocker --- filestore/filelock.go | 58 ++++++++++++++++++++++++++++++++++++++ filestore/filelock_test.go | 35 +++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 filestore/filelock.go create mode 100644 filestore/filelock_test.go diff --git a/filestore/filelock.go b/filestore/filelock.go new file mode 100644 index 0000000..8184a5d --- /dev/null +++ b/filestore/filelock.go @@ -0,0 +1,58 @@ +package filestore + +import ( + "os" + "path/filepath" + + "github.com/nightlyone/lockfile" + "github.com/tus/tusd" +) + +type FileLocker struct { + // Relative or absolute path to store the locks in. + Path string +} + +func (locker FileLocker) LockUpload(id string) error { + lock, err := locker.newLock(id) + if err != nil { + return err + } + + err = lock.TryLock() + if err == lockfile.ErrBusy { + return tusd.ErrFileLocked + } + + return err +} + +func (locker FileLocker) UnlockUpload(id string) error { + lock, err := locker.newLock(id) + if err != nil { + return err + } + + err = lock.Unlock() + + // A "no such file or directory" will be returned if no lockfile was found. + // Since this means that the file has never been locked, we drop the error + // and continue as if nothing happend. + if os.IsNotExist(err) { + err = nil + } + + return nil +} + +func (locker FileLocker) newLock(id string) (lockfile.Lockfile, error) { + path, err := filepath.Abs(locker.Path + "/" + id + ".lock") + if err != nil { + return lockfile.Lockfile(""), err + } + + // We use Lockfile directly instead of lockfile.New to bypass the unnecessary + // check whether the provided path is absolute since we just resolved it + // on our own. + return lockfile.Lockfile(path), nil +} diff --git a/filestore/filelock_test.go b/filestore/filelock_test.go new file mode 100644 index 0000000..71c5c97 --- /dev/null +++ b/filestore/filelock_test.go @@ -0,0 +1,35 @@ +package filestore + +import ( + "io/ioutil" + "testing" + + "github.com/tus/tusd" + "github.com/tus/tusd/lockingstore" +) + +func TestFileLocker(t *testing.T) { + dir, err := ioutil.TempDir("", "tusd-file-locker") + if err != nil { + t.Fatal(err) + } + + var locker lockingstore.Locker + locker = FileLocker{dir} + + if err := locker.LockUpload("one"); err != nil { + t.Errorf("unexpected error when locking file: %s", err) + } + + if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { + t.Errorf("expected error when locking locked file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file again: %s", err) + } +} From b4c0df187e713ec1a034fefa981ead4ef5f5c6b3 Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 19 Dec 2015 00:21:34 +0100 Subject: [PATCH 10/17] Provide easier way for locking file storage --- cmd/tusd/main.go | 4 +--- filestore/filestore.go | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/cmd/tusd/main.go b/cmd/tusd/main.go index 474ec4d..07c09e5 100644 --- a/cmd/tusd/main.go +++ b/cmd/tusd/main.go @@ -44,9 +44,7 @@ func main() { } var store tusd.DataStore - store = filestore.FileStore{ - Path: dir, - } + store = filestore.New(dir) if storeSize > 0 { store = limitedstore.New(storeSize, store) diff --git a/filestore/filestore.go b/filestore/filestore.go index e524724..56e52e4 100644 --- a/filestore/filestore.go +++ b/filestore/filestore.go @@ -15,6 +15,7 @@ import ( "os" "github.com/tus/tusd" + "github.com/tus/tusd/lockingstore" "github.com/tus/tusd/uid" ) @@ -24,10 +25,25 @@ var defaultFilePerm = os.FileMode(0775) // methods. type FileStore struct { // Relative or absolute path to store files in. FileStore does not check - // whether the path exists, you os.MkdirAll in this case on your own. + // whether the path exists, use os.MkdirAll in this case on your own. Path string } +// New creates a new file based storage backend. The directory specified will +// be used as the only storage entry. This method does not check +// whether the path exists, use os.MkdirAll to ensure. +// In addition, a locking mechanism is provided using lockingstore.LockingStore +// and FileLocker. +func New(path string) tusd.DataStore { + store := FileStore{path} + locker := FileLocker{path} + + return lockingstore.LockingStore{ + DataStore: &store, + Locker: &locker, + } +} + func (store FileStore) NewUpload(info tusd.FileInfo) (id string, err error) { id = uid.Uid() info.ID = id From 14285e971d88b9f822d311f8c2ecbef74dbc08b8 Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 25 Dec 2015 21:33:58 +0100 Subject: [PATCH 11/17] Add documentation for FileLocker --- filestore/filelock.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/filestore/filelock.go b/filestore/filelock.go index 8184a5d..8022820 100644 --- a/filestore/filelock.go +++ b/filestore/filelock.go @@ -8,6 +8,13 @@ import ( "github.com/tus/tusd" ) +// FileLocker provides an exclusive upload locking mechansim using lock files +// which are stored on disk. Each of them stores the PID of the process which +// aquired the lock. This allows locks to be automatically freed when a process +// is unable to release it on its own because the process is not alive anymore. +// +// Please consult the package lockingstore for instructions on how to use this +// locker. type FileLocker struct { // Relative or absolute path to store the locks in. Path string From cfabbf5ffb511b01b2c8c68bc1689c107c490eca Mon Sep 17 00:00:00 2001 From: Marius Date: Fri, 25 Dec 2015 22:33:27 +0100 Subject: [PATCH 12/17] Extract tests into tusd_test package --- concat_test.go | 4 +++- cors_test.go | 4 +++- get_test.go | 4 +++- handler_test.go | 4 +++- head_test.go | 4 +++- options_test.go | 4 +++- patch_test.go | 4 +++- post_test.go | 4 +++- terminate_test.go | 4 +++- 9 files changed, 27 insertions(+), 9 deletions(-) diff --git a/concat_test.go b/concat_test.go index 250ce6e..093107f 100644 --- a/concat_test.go +++ b/concat_test.go @@ -1,4 +1,4 @@ -package tusd +package tusd_test import ( "io" @@ -7,6 +7,8 @@ import ( "reflect" "strings" "testing" + + . "github.com/tus/tusd" ) type concatPartialStore struct { diff --git a/cors_test.go b/cors_test.go index c1b24ae..d1f0209 100644 --- a/cors_test.go +++ b/cors_test.go @@ -1,8 +1,10 @@ -package tusd +package tusd_test import ( "net/http" "testing" + + . "github.com/tus/tusd" ) func TestCORS(t *testing.T) { diff --git a/get_test.go b/get_test.go index 17907e1..5310854 100644 --- a/get_test.go +++ b/get_test.go @@ -1,4 +1,4 @@ -package tusd +package tusd_test import ( "io" @@ -6,6 +6,8 @@ import ( "os" "strings" "testing" + + . "github.com/tus/tusd" ) type getStore struct { diff --git a/handler_test.go b/handler_test.go index 6119b1b..54172b1 100644 --- a/handler_test.go +++ b/handler_test.go @@ -1,4 +1,4 @@ -package tusd +package tusd_test import ( "io" @@ -7,6 +7,8 @@ import ( "os" "strings" "testing" + + . "github.com/tus/tusd" ) type zeroStore struct{} diff --git a/head_test.go b/head_test.go index 9b809af..9b63b59 100644 --- a/head_test.go +++ b/head_test.go @@ -1,9 +1,11 @@ -package tusd +package tusd_test import ( "net/http" "os" "testing" + + . "github.com/tus/tusd" ) type headStore struct { diff --git a/options_test.go b/options_test.go index 3a718e2..8c7289b 100644 --- a/options_test.go +++ b/options_test.go @@ -1,8 +1,10 @@ -package tusd +package tusd_test import ( "net/http" "testing" + + . "github.com/tus/tusd" ) func TestOptions(t *testing.T) { diff --git a/patch_test.go b/patch_test.go index 7e24c72..1777e4d 100644 --- a/patch_test.go +++ b/patch_test.go @@ -1,4 +1,4 @@ -package tusd +package tusd_test import ( "io" @@ -7,6 +7,8 @@ import ( "os" "strings" "testing" + + . "github.com/tus/tusd" ) type patchStore struct { diff --git a/post_test.go b/post_test.go index 4606980..4010bc1 100644 --- a/post_test.go +++ b/post_test.go @@ -1,8 +1,10 @@ -package tusd +package tusd_test import ( "net/http" "testing" + + . "github.com/tus/tusd" ) type postStore struct { diff --git a/terminate_test.go b/terminate_test.go index efca8e1..a4a3935 100644 --- a/terminate_test.go +++ b/terminate_test.go @@ -1,8 +1,10 @@ -package tusd +package tusd_test import ( "net/http" "testing" + + . "github.com/tus/tusd" ) type terminateStore struct { From f6a5530df833fd62f54dc60943f141624b068d90 Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 26 Dec 2015 21:23:09 +0100 Subject: [PATCH 13/17] Integrate LockingStore into UnroutedHandler The reason behind this drastic move was that sometimes a lock needs to persist across multiple calls to a DataStore. For example, when a PATCH request is received, following behaviour is wanted: * Obtain lock (LockUpload) * Get offset, size etc (GetInfo) * Write data (WriteChunk) * Release lock (UnlockUpload) However, before this change, the lock would be release and then obtained again after the GetInfo and before the WriteChunk call. This resulted in an inefficient resource usage and even a possible race condition. The effects of this change was: * FileLocker is now directly integrated into FileStore and not sperarated * LockingStore and the entire package has been removed * MemoryLocker has been moved into its very own package --- datastore.go | 21 ++++ filestore/filelock.go | 65 ------------ filestore/filelock_test.go | 35 ------- filestore/filestore.go | 77 ++++++++++++--- filestore/filestore_test.go | 26 +++++ lockingstore/lockingstore.go | 99 ------------------- lockingstore/lockingstore_test.go | 82 --------------- lockingstore/memorylocker_test.go | 28 ------ .../memorylocker.go | 20 +++- memorylocker/memorylocker_test.go | 50 ++++++++++ patch_test.go | 84 ++++++++++++++++ unrouted_handler.go | 37 +++++++ 12 files changed, 296 insertions(+), 328 deletions(-) delete mode 100644 filestore/filelock.go delete mode 100644 filestore/filelock_test.go delete mode 100644 lockingstore/lockingstore.go delete mode 100644 lockingstore/lockingstore_test.go delete mode 100644 lockingstore/memorylocker_test.go rename {lockingstore => memorylocker}/memorylocker.go (53%) create mode 100644 memorylocker/memorylocker_test.go diff --git a/datastore.go b/datastore.go index 8c335dc..c891c9f 100644 --- a/datastore.go +++ b/datastore.go @@ -55,3 +55,24 @@ type DataStore interface { // and writing, must return os.ErrNotExist or similar. Terminate(id string) error } + +// LockerDataStore is the interface required for custom lock persisting mechanisms. +// Common ways to store this information is in memory, on disk or using an +// external service, such as ZooKeeper. +// When multiple processes are attempting to access an upload, whether it be +// by reading or writing, a syncronization mechanism is required to prevent +// data corruption, especially to ensure correct offset values and the proper +// order of chunks inside a single upload. +type LockerDataStore interface { + DataStore + + // LockUpload attempts to obtain an exclusive lock for the upload specified + // by its id. + // If this operation fails because the resource is already locked, the + // tusd.ErrFileLocked must be returned. If no error is returned, the attempt + // is consider to be successful and the upload to be locked until UnlockUpload + // is invoked for the same upload. + LockUpload(id string) error + // UnlockUpload releases an existing lock for the given upload. + UnlockUpload(id string) error +} diff --git a/filestore/filelock.go b/filestore/filelock.go deleted file mode 100644 index 8022820..0000000 --- a/filestore/filelock.go +++ /dev/null @@ -1,65 +0,0 @@ -package filestore - -import ( - "os" - "path/filepath" - - "github.com/nightlyone/lockfile" - "github.com/tus/tusd" -) - -// FileLocker provides an exclusive upload locking mechansim using lock files -// which are stored on disk. Each of them stores the PID of the process which -// aquired the lock. This allows locks to be automatically freed when a process -// is unable to release it on its own because the process is not alive anymore. -// -// Please consult the package lockingstore for instructions on how to use this -// locker. -type FileLocker struct { - // Relative or absolute path to store the locks in. - Path string -} - -func (locker FileLocker) LockUpload(id string) error { - lock, err := locker.newLock(id) - if err != nil { - return err - } - - err = lock.TryLock() - if err == lockfile.ErrBusy { - return tusd.ErrFileLocked - } - - return err -} - -func (locker FileLocker) UnlockUpload(id string) error { - lock, err := locker.newLock(id) - if err != nil { - return err - } - - err = lock.Unlock() - - // A "no such file or directory" will be returned if no lockfile was found. - // Since this means that the file has never been locked, we drop the error - // and continue as if nothing happend. - if os.IsNotExist(err) { - err = nil - } - - return nil -} - -func (locker FileLocker) newLock(id string) (lockfile.Lockfile, error) { - path, err := filepath.Abs(locker.Path + "/" + id + ".lock") - if err != nil { - return lockfile.Lockfile(""), err - } - - // We use Lockfile directly instead of lockfile.New to bypass the unnecessary - // check whether the provided path is absolute since we just resolved it - // on our own. - return lockfile.Lockfile(path), nil -} diff --git a/filestore/filelock_test.go b/filestore/filelock_test.go deleted file mode 100644 index 71c5c97..0000000 --- a/filestore/filelock_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package filestore - -import ( - "io/ioutil" - "testing" - - "github.com/tus/tusd" - "github.com/tus/tusd/lockingstore" -) - -func TestFileLocker(t *testing.T) { - dir, err := ioutil.TempDir("", "tusd-file-locker") - if err != nil { - t.Fatal(err) - } - - var locker lockingstore.Locker - locker = FileLocker{dir} - - if err := locker.LockUpload("one"); err != nil { - t.Errorf("unexpected error when locking file: %s", err) - } - - if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { - t.Errorf("expected error when locking locked file: %s", err) - } - - if err := locker.UnlockUpload("one"); err != nil { - t.Errorf("unexpected error when unlocking file: %s", err) - } - - if err := locker.UnlockUpload("one"); err != nil { - t.Errorf("unexpected error when unlocking file again: %s", err) - } -} diff --git a/filestore/filestore.go b/filestore/filestore.go index 56e52e4..268c28c 100644 --- a/filestore/filestore.go +++ b/filestore/filestore.go @@ -6,6 +6,13 @@ // `[id].bin` files contain the raw binary data uploaded. // No cleanup is performed so you may want to run a cronjob to ensure your disk // is not filled up with old and finished uploads. +// +// In addition, it provides an exclusive upload locking mechansim using lock files +// which are stored on disk. Each of them stores the PID of the process which +// aquired the lock. This allows locks to be automatically freed when a process +// is unable to release it on its own because the process is not alive anymore. +// For more information, consult the documentation for tusd.LockerDataStore +// interface, which is implemented by FileStore package filestore import ( @@ -13,10 +20,12 @@ import ( "io" "io/ioutil" "os" + "path/filepath" "github.com/tus/tusd" - "github.com/tus/tusd/lockingstore" "github.com/tus/tusd/uid" + + "github.com/nightlyone/lockfile" ) var defaultFilePerm = os.FileMode(0775) @@ -32,16 +41,9 @@ type FileStore struct { // New creates a new file based storage backend. The directory specified will // be used as the only storage entry. This method does not check // whether the path exists, use os.MkdirAll to ensure. -// In addition, a locking mechanism is provided using lockingstore.LockingStore -// and FileLocker. -func New(path string) tusd.DataStore { - store := FileStore{path} - locker := FileLocker{path} - - return lockingstore.LockingStore{ - DataStore: &store, - Locker: &locker, - } +// In addition, a locking mechanism is provided. +func New(path string) FileStore { + return FileStore{path} } func (store FileStore) NewUpload(info tusd.FileInfo) (id string, err error) { @@ -100,17 +102,62 @@ func (store FileStore) Terminate(id string) error { return nil } -// Return the path to the .bin storing the binary data +func (store FileStore) LockUpload(id string) error { + lock, err := store.newLock(id) + if err != nil { + return err + } + + err = lock.TryLock() + if err == lockfile.ErrBusy { + return tusd.ErrFileLocked + } + + return err +} + +func (store FileStore) UnlockUpload(id string) error { + lock, err := store.newLock(id) + if err != nil { + return err + } + + err = lock.Unlock() + + // A "no such file or directory" will be returned if no lockfile was found. + // Since this means that the file has never been locked, we drop the error + // and continue as if nothing happend. + if os.IsNotExist(err) { + err = nil + } + + return nil +} + +// newLock contructs a new Lockfile instance. +func (store FileStore) newLock(id string) (lockfile.Lockfile, error) { + path, err := filepath.Abs(store.Path + "/" + id + ".lock") + if err != nil { + return lockfile.Lockfile(""), err + } + + // We use Lockfile directly instead of lockfile.New to bypass the unnecessary + // check whether the provided path is absolute since we just resolved it + // on our own. + return lockfile.Lockfile(path), nil +} + +// binPath returns the path to the .bin storing the binary data. func (store FileStore) binPath(id string) string { return store.Path + "/" + id + ".bin" } -// Return the path to the .info file storing the file's info +// infoPath returns the path to the .info file storing the file's info. func (store FileStore) infoPath(id string) string { return store.Path + "/" + id + ".info" } -// Update the entire information. Everything will be overwritten. +// writeInfo updates the entire information. Everything will be overwritten. func (store FileStore) writeInfo(id string, info tusd.FileInfo) error { data, err := json.Marshal(info) if err != nil { @@ -119,7 +166,7 @@ func (store FileStore) writeInfo(id string, info tusd.FileInfo) error { return ioutil.WriteFile(store.infoPath(id), data, defaultFilePerm) } -// Update the .info file using the new upload. +// setOffset updates the .info file to match the new offset. func (store FileStore) setOffset(id string, offset int64) error { info, err := store.GetInfo(id) if err != nil { diff --git a/filestore/filestore_test.go b/filestore/filestore_test.go index 74c4dfc..06da9eb 100644 --- a/filestore/filestore_test.go +++ b/filestore/filestore_test.go @@ -95,3 +95,29 @@ func TestFilestore(t *testing.T) { t.Fatal("expected os.ErrIsNotExist") } } + +func TestFileLocker(t *testing.T) { + dir, err := ioutil.TempDir("", "tusd-file-locker") + if err != nil { + t.Fatal(err) + } + + var locker tusd.LockerDataStore + locker = FileStore{dir} + + if err := locker.LockUpload("one"); err != nil { + t.Errorf("unexpected error when locking file: %s", err) + } + + if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { + t.Errorf("expected error when locking locked file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file again: %s", err) + } +} diff --git a/lockingstore/lockingstore.go b/lockingstore/lockingstore.go deleted file mode 100644 index 13019af..0000000 --- a/lockingstore/lockingstore.go +++ /dev/null @@ -1,99 +0,0 @@ -// Package lockingstore manages concurrent access to a single upload. -// -// When multiple processes are attempting to access an upload, whether it be -// by reading or writing, a syncronization mechanism is required to prevent -// data corruption, especially to ensure correct offset values and the proper -// order of chunks inside a single upload. -// -// This package wrappes an existing data storage and only allows a single access -// at a time by using an exclusive locking mechanism. -package lockingstore - -import ( - "io" - - "github.com/tus/tusd" -) - -// Locker is the interface required for custom lock persisting mechanisms. -// Common ways to store this information is in memory, on disk or using an -// external service, such as ZooKeeper. -type Locker interface { - // LockUpload attempts to obtain an exclusive lock for the upload specified - // by its id. - // If this operation fails because the resource is already locked, the - // tusd.ErrFileLocked must be returned. If no error is returned, the attempt - // is consider to be successful and the upload to be locked until UnlockUpload - // is invoked for the same upload. - LockUpload(id string) error - // UnlockUpload releases an existing lock for the given upload. - UnlockUpload(id string) error -} - -// LockingStore wraps an existing data storage and catches all operation. -// Before passing the method calls to the underlying backend, locks are required -// to be obtained. -type LockingStore struct { - // The underlying data storage to which the operation will be passed if an - // upload is not locked. - tusd.DataStore - // The custom locking persisting mechanism used for obtaining and releasing - // locks. - Locker Locker -} - -func (store LockingStore) WriteChunk(id string, offset int64, src io.Reader) (n int64, err error) { - if err := store.Locker.LockUpload(id); err != nil { - return 0, err - } - - defer func() { - if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { - err = unlockErr - } - }() - - return store.DataStore.WriteChunk(id, offset, src) -} - -func (store LockingStore) GetInfo(id string) (info tusd.FileInfo, err error) { - if err := store.Locker.LockUpload(id); err != nil { - return info, err - } - - defer func() { - if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { - err = unlockErr - } - }() - - return store.DataStore.GetInfo(id) -} - -func (store LockingStore) GetReader(id string) (src io.Reader, err error) { - if err := store.Locker.LockUpload(id); err != nil { - return nil, err - } - - defer func() { - if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { - err = unlockErr - } - }() - - return store.DataStore.GetReader(id) -} - -func (store LockingStore) Terminate(id string) (err error) { - if err := store.Locker.LockUpload(id); err != nil { - return err - } - - defer func() { - if unlockErr := store.Locker.UnlockUpload(id); unlockErr != nil { - err = unlockErr - } - }() - - return store.DataStore.Terminate(id) -} diff --git a/lockingstore/lockingstore_test.go b/lockingstore/lockingstore_test.go deleted file mode 100644 index 4fd3abc..0000000 --- a/lockingstore/lockingstore_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package lockingstore_test - -import ( - "io" - "testing" - - "github.com/tus/tusd" - . "github.com/tus/tusd/lockingstore" -) - -type store struct { - calls int -} - -func (store *store) NewUpload(info tusd.FileInfo) (string, error) { - return "", nil -} -func (store *store) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { - store.calls += 1 - return 0, nil -} - -func (store *store) GetInfo(id string) (tusd.FileInfo, error) { - store.calls += 1 - return tusd.FileInfo{}, nil -} - -func (store *store) GetReader(id string) (io.Reader, error) { - store.calls += 1 - return nil, nil -} - -func (store *store) Terminate(id string) error { - store.calls += 1 - return nil -} - -type locker struct { - lockCalls int - unlockCalls int -} - -func (locker *locker) LockUpload(id string) error { - locker.lockCalls += 1 - if id == "no" { - return tusd.ErrFileLocked - } - return nil -} - -func (locker *locker) UnlockUpload(id string) error { - locker.unlockCalls += 1 - return nil -} - -func TestLockingStore(t *testing.T) { - locker := new(locker) - store := new(store) - lstore := LockingStore{ - DataStore: store, - Locker: locker, - } - - lstore.NewUpload(tusd.FileInfo{}) - lstore.WriteChunk("", 0, nil) - lstore.GetInfo("") - lstore.GetReader("") - lstore.Terminate("") - - lstore.WriteChunk("no", 0, nil) - lstore.GetInfo("no") - lstore.GetReader("no") - lstore.Terminate("no") - - if locker.lockCalls != 8 { - t.Error("expected 8 calls to LockUpload, but got %d", locker.lockCalls) - } - - if locker.unlockCalls != 4 { - t.Error("expected 8 calls to UnlockUpload, but got %d", locker.unlockCalls) - } -} diff --git a/lockingstore/memorylocker_test.go b/lockingstore/memorylocker_test.go deleted file mode 100644 index d14fc7b..0000000 --- a/lockingstore/memorylocker_test.go +++ /dev/null @@ -1,28 +0,0 @@ -package lockingstore - -import ( - "testing" - - "github.com/tus/tusd" -) - -func TestMemoryLocker(t *testing.T) { - var locker Locker - locker = NewMemoryLocker() - - if err := locker.LockUpload("one"); err != nil { - t.Errorf("unexpected error when locking file: %s", err) - } - - if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { - t.Errorf("expected error when locking locked file: %s", err) - } - - if err := locker.UnlockUpload("one"); err != nil { - t.Errorf("unexpected error when unlocking file: %s", err) - } - - if err := locker.UnlockUpload("one"); err != nil { - t.Errorf("unexpected error when unlocking file again: %s", err) - } -} diff --git a/lockingstore/memorylocker.go b/memorylocker/memorylocker.go similarity index 53% rename from lockingstore/memorylocker.go rename to memorylocker/memorylocker.go index d15ecc0..a565a7e 100644 --- a/lockingstore/memorylocker.go +++ b/memorylocker/memorylocker.go @@ -1,4 +1,14 @@ -package lockingstore +// Package memorylocker provides an in-memory locking mechanism +// +// When multiple processes are attempting to access an upload, whether it be +// by reading or writing, a syncronization mechanism is required to prevent +// data corruption, especially to ensure correct offset values and the proper +// order of chunks inside a single upload. +// +// MemoryLocker persists locks using memory and therefore allowing a simple and +// cheap mechansim. Locks will only exist as long as this object is kept in +// reference and will be erased if the program exits. +package memorylocker import ( "github.com/tus/tusd" @@ -8,13 +18,15 @@ import ( // cheap mechansim. Locks will only exist as long as this object is kept in // reference and will be erased if the program exits. type MemoryLocker struct { + tusd.DataStore locks map[string]bool } -// New creates a new lock memory persistor. -func NewMemoryLocker() *MemoryLocker { +// New creates a new lock memory wrapper around the provided storage. +func NewMemoryLocker(store tusd.DataStore) *MemoryLocker { return &MemoryLocker{ - locks: make(map[string]bool), + DataStore: store, + locks: make(map[string]bool), } } diff --git a/memorylocker/memorylocker_test.go b/memorylocker/memorylocker_test.go new file mode 100644 index 0000000..6f832b8 --- /dev/null +++ b/memorylocker/memorylocker_test.go @@ -0,0 +1,50 @@ +package memorylocker + +import ( + "io" + "testing" + + "github.com/tus/tusd" +) + +type zeroStore struct{} + +func (store zeroStore) NewUpload(info tusd.FileInfo) (string, error) { + return "", nil +} +func (store zeroStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { + return 0, nil +} + +func (store zeroStore) GetInfo(id string) (tusd.FileInfo, error) { + return tusd.FileInfo{}, nil +} + +func (store zeroStore) GetReader(id string) (io.Reader, error) { + return nil, tusd.ErrNotImplemented +} + +func (store zeroStore) Terminate(id string) error { + return tusd.ErrNotImplemented +} + +func TestMemoryLocker(t *testing.T) { + var locker tusd.LockerDataStore + locker = NewMemoryLocker(&zeroStore{}) + + if err := locker.LockUpload("one"); err != nil { + t.Errorf("unexpected error when locking file: %s", err) + } + + if err := locker.LockUpload("one"); err != tusd.ErrFileLocked { + t.Errorf("expected error when locking locked file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file: %s", err) + } + + if err := locker.UnlockUpload("one"); err != nil { + t.Errorf("unexpected error when unlocking file again: %s", err) + } +} diff --git a/patch_test.go b/patch_test.go index 1777e4d..3f2c4ca 100644 --- a/patch_test.go +++ b/patch_test.go @@ -206,3 +206,87 @@ func TestPatchOverflow(t *testing.T) { Code: http.StatusNoContent, }).Run(handler, t) } + +const ( + LOCK = iota + INFO + WRITE + UNLOCK + END +) + +type lockingPatchStore struct { + zeroStore + callOrder chan int +} + +func (s lockingPatchStore) GetInfo(id string) (FileInfo, error) { + s.callOrder <- INFO + + return FileInfo{ + Offset: 0, + Size: 20, + }, nil +} + +func (s lockingPatchStore) WriteChunk(id string, offset int64, src io.Reader) (int64, error) { + s.callOrder <- WRITE + + return 5, nil +} + +func (s lockingPatchStore) LockUpload(id string) error { + s.callOrder <- LOCK + return nil +} + +func (s lockingPatchStore) UnlockUpload(id string) error { + s.callOrder <- UNLOCK + return nil +} + +func TestLockingPatch(t *testing.T) { + callOrder := make(chan int, 10) + + handler, _ := NewHandler(Config{ + DataStore: lockingPatchStore{ + callOrder: callOrder, + }, + }) + + (&httpTest{ + Name: "Uploading to locking store", + Method: "PATCH", + URL: "yes", + ReqHeader: map[string]string{ + "Tus-Resumable": "1.0.0", + "Content-Type": "application/offset+octet-stream", + "Upload-Offset": "0", + }, + ReqBody: strings.NewReader("hello"), + Code: http.StatusNoContent, + }).Run(handler, t) + + callOrder <- END + close(callOrder) + + if <-callOrder != LOCK { + t.Error("expected call to LockUpload") + } + + if <-callOrder != INFO { + t.Error("expected call to GetInfo") + } + + if <-callOrder != WRITE { + t.Error("expected call to WriteChunk") + } + + if <-callOrder != UNLOCK { + t.Error("expected call to UnlockUpload") + } + + if <-callOrder != END { + t.Error("expected no more calls to happen") + } +} diff --git a/unrouted_handler.go b/unrouted_handler.go index a17249b..409fb36 100644 --- a/unrouted_handler.go +++ b/unrouted_handler.go @@ -257,6 +257,16 @@ func (handler *UnroutedHandler) HeadFile(w http.ResponseWriter, r *http.Request) handler.sendError(w, r, err) return } + + if locker, ok := handler.dataStore.(LockerDataStore); ok { + if err := locker.LockUpload(id); err != nil { + handler.sendError(w, r, err) + return + } + + defer locker.UnlockUpload(id) + } + info, err := handler.dataStore.GetInfo(id) if err != nil { handler.sendError(w, r, err) @@ -308,6 +318,15 @@ func (handler *UnroutedHandler) PatchFile(w http.ResponseWriter, r *http.Request return } + if locker, ok := handler.dataStore.(LockerDataStore); ok { + if err := locker.LockUpload(id); err != nil { + handler.sendError(w, r, err) + return + } + + defer locker.UnlockUpload(id) + } + info, err := handler.dataStore.GetInfo(id) if err != nil { handler.sendError(w, r, err) @@ -370,6 +389,15 @@ func (handler *UnroutedHandler) GetFile(w http.ResponseWriter, r *http.Request) return } + if locker, ok := handler.dataStore.(LockerDataStore); ok { + if err := locker.LockUpload(id); err != nil { + handler.sendError(w, r, err) + return + } + + defer locker.UnlockUpload(id) + } + info, err := handler.dataStore.GetInfo(id) if err != nil { handler.sendError(w, r, err) @@ -407,6 +435,15 @@ func (handler *UnroutedHandler) DelFile(w http.ResponseWriter, r *http.Request) return } + if locker, ok := handler.dataStore.(LockerDataStore); ok { + if err := locker.LockUpload(id); err != nil { + handler.sendError(w, r, err) + return + } + + defer locker.UnlockUpload(id) + } + err = handler.dataStore.Terminate(id) if err != nil { handler.sendError(w, r, err) From 4ee2a09510cfb53dbd3ddaf632146d3581709c7d Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 26 Dec 2015 22:28:36 +0100 Subject: [PATCH 14/17] Use Go's vendoring for dependencies --- .gitmodules | 3 +++ vendor/github.com/nightlyone/lockfile | 1 + 2 files changed, 4 insertions(+) create mode 100644 .gitmodules create mode 160000 vendor/github.com/nightlyone/lockfile diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 0000000..005d3d1 --- /dev/null +++ b/.gitmodules @@ -0,0 +1,3 @@ +[submodule "vendor/github.com/nightlyone/lockfile"] + path = vendor/github.com/nightlyone/lockfile + url = git@github.com:nightlyone/lockfile.git diff --git a/vendor/github.com/nightlyone/lockfile b/vendor/github.com/nightlyone/lockfile new file mode 160000 index 0000000..2275425 --- /dev/null +++ b/vendor/github.com/nightlyone/lockfile @@ -0,0 +1 @@ +Subproject commit 22754258d2b05a18f75f228588041de6fe9fdcc8 From 28eb393dc5b320bff602a85c869248300e96bac3 Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 26 Dec 2015 22:34:25 +0100 Subject: [PATCH 15/17] Use HTTPS- instead of SSH url for submodule --- .gitmodules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitmodules b/.gitmodules index 005d3d1..4160413 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,3 @@ [submodule "vendor/github.com/nightlyone/lockfile"] path = vendor/github.com/nightlyone/lockfile - url = git@github.com:nightlyone/lockfile.git + url = https://github.com/nightlyone/lockfile.git From f96e2614fc420c31da63fe3fe6f140f58be72094 Mon Sep 17 00:00:00 2001 From: Marius Date: Sat, 26 Dec 2015 22:36:38 +0100 Subject: [PATCH 16/17] Test against Go 1.5 and 1.6 on Travis --- .travis.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.travis.yml b/.travis.yml index dd08616..9680be4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,8 +3,14 @@ go: - 1.2 - 1.3 - 1.4 +- 1.5 +- 1.6 - tip +env: + global: + - GO15VENDOREXPERIMENT=1 + matrix: allow_failures: - go: tip From 4af7434c10960c00a91c86d5f13fe41dbb806041 Mon Sep 17 00:00:00 2001 From: Marius Date: Sun, 27 Dec 2015 00:44:02 +0100 Subject: [PATCH 17/17] Move terminating method in own interface In addition, the DELETE handler is only provided if the TerminaterDataStore interface is implemented. --- cmd/tusd/main.go | 2 +- datastore.go | 8 ++++++++ handler.go | 6 +++++- handler_test.go | 4 ---- limitedstore/limitedstore.go | 20 +++++++++++--------- memorylocker/memorylocker_test.go | 4 ---- options_test.go | 2 +- terminate_test.go | 26 ++++++++++++++++++++++++++ unrouted_handler.go | 19 +++++++++++++++++-- 9 files changed, 69 insertions(+), 22 deletions(-) diff --git a/cmd/tusd/main.go b/cmd/tusd/main.go index 07c09e5..de7ffe9 100644 --- a/cmd/tusd/main.go +++ b/cmd/tusd/main.go @@ -43,7 +43,7 @@ func main() { stderr.Fatalf("Unable to ensure directory exists: %s", err) } - var store tusd.DataStore + var store tusd.TerminaterDataStore store = filestore.New(dir) if storeSize > 0 { diff --git a/datastore.go b/datastore.go index c891c9f..e8ae558 100644 --- a/datastore.go +++ b/datastore.go @@ -51,6 +51,14 @@ type DataStore interface { // If the returned reader also implements the io.Closer interface, the // Close() method will be invoked once everything has been read. GetReader(id string) (io.Reader, error) +} + +// TerminaterDataStore is the interface which must be implemented by DataStores +// if they want to receive DELETE requests using the Handler. If this interface +// is not implemented, no request handler for this method is attached. +type TerminaterDataStore interface { + DataStore + // Terminate an upload so any further requests to the resource, both reading // and writing, must return os.ErrNotExist or similar. Terminate(id string) error diff --git a/handler.go b/handler.go index aafb1e2..0aae179 100644 --- a/handler.go +++ b/handler.go @@ -39,9 +39,13 @@ func NewHandler(config Config) (*Handler, error) { mux.Post("", http.HandlerFunc(handler.PostFile)) mux.Head(":id", http.HandlerFunc(handler.HeadFile)) mux.Get(":id", http.HandlerFunc(handler.GetFile)) - mux.Del(":id", http.HandlerFunc(handler.DelFile)) mux.Add("PATCH", ":id", http.HandlerFunc(handler.PatchFile)) + // Only attach the DELETE handler if the Terminate() method is provided + if _, ok := config.DataStore.(TerminaterDataStore); ok { + mux.Del(":id", http.HandlerFunc(handler.DelFile)) + } + return routedHandler, nil } diff --git a/handler_test.go b/handler_test.go index 54172b1..d919f23 100644 --- a/handler_test.go +++ b/handler_test.go @@ -28,10 +28,6 @@ func (store zeroStore) GetReader(id string) (io.Reader, error) { return nil, ErrNotImplemented } -func (store zeroStore) Terminate(id string) error { - return ErrNotImplemented -} - type httpTest struct { Name string diff --git a/limitedstore/limitedstore.go b/limitedstore/limitedstore.go index 7b6da8c..8a56a45 100644 --- a/limitedstore/limitedstore.go +++ b/limitedstore/limitedstore.go @@ -22,7 +22,7 @@ import ( type LimitedStore struct { StoreSize int64 - tusd.DataStore + tusd.TerminaterDataStore uploads map[string]int64 usedSize int64 @@ -42,13 +42,15 @@ func (p pairlist) Len() int { return len(p) } func (p pairlist) Swap(i, j int) { p[i], p[j] = p[j], p[i] } func (p pairlist) Less(i, j int) bool { return p[i].value < p[j].value } -// Create a new limited store with the given size as the maximum storage size -func New(storeSize int64, dataStore tusd.DataStore) *LimitedStore { +// New creates a new limited store with the given size as the maximum storage +// size. The wrapped data store needs to implement the TerminaterDataStore +// interface, in order to provide the required Terminate method. +func New(storeSize int64, dataStore tusd.TerminaterDataStore) *LimitedStore { return &LimitedStore{ - StoreSize: storeSize, - DataStore: dataStore, - uploads: make(map[string]int64), - mutex: new(sync.Mutex), + StoreSize: storeSize, + TerminaterDataStore: dataStore, + uploads: make(map[string]int64), + mutex: new(sync.Mutex), } } @@ -60,7 +62,7 @@ func (store *LimitedStore) NewUpload(info tusd.FileInfo) (string, error) { return "", err } - id, err := store.DataStore.NewUpload(info) + id, err := store.TerminaterDataStore.NewUpload(info) if err != nil { return "", err } @@ -79,7 +81,7 @@ func (store *LimitedStore) Terminate(id string) error { } func (store *LimitedStore) terminate(id string) error { - err := store.DataStore.Terminate(id) + err := store.TerminaterDataStore.Terminate(id) if err != nil { return err } diff --git a/memorylocker/memorylocker_test.go b/memorylocker/memorylocker_test.go index 6f832b8..1f91573 100644 --- a/memorylocker/memorylocker_test.go +++ b/memorylocker/memorylocker_test.go @@ -24,10 +24,6 @@ func (store zeroStore) GetReader(id string) (io.Reader, error) { return nil, tusd.ErrNotImplemented } -func (store zeroStore) Terminate(id string) error { - return tusd.ErrNotImplemented -} - func TestMemoryLocker(t *testing.T) { var locker tusd.LockerDataStore locker = NewMemoryLocker(&zeroStore{}) diff --git a/options_test.go b/options_test.go index 8c7289b..88f73e9 100644 --- a/options_test.go +++ b/options_test.go @@ -17,7 +17,7 @@ func TestOptions(t *testing.T) { Method: "OPTIONS", Code: http.StatusNoContent, ResHeader: map[string]string{ - "Tus-Extension": "creation,concatenation,termination", + "Tus-Extension": "creation,concatenation", "Tus-Version": "1.0.0", "Tus-Resumable": "1.0.0", "Tus-Max-Size": "400", diff --git a/terminate_test.go b/terminate_test.go index a4a3935..bf858cc 100644 --- a/terminate_test.go +++ b/terminate_test.go @@ -26,6 +26,16 @@ func TestTerminate(t *testing.T) { }, }) + (&httpTest{ + Name: "Successful OPTIONS request", + Method: "OPTIONS", + URL: "", + ResHeader: map[string]string{ + "Tus-Extension": "creation,concatenation,termination", + }, + Code: http.StatusNoContent, + }).Run(handler, t) + (&httpTest{ Name: "Successful request", Method: "DELETE", @@ -36,3 +46,19 @@ func TestTerminate(t *testing.T) { Code: http.StatusNoContent, }).Run(handler, t) } + +func TestTerminateNotImplemented(t *testing.T) { + handler, _ := NewHandler(Config{ + DataStore: zeroStore{}, + }) + + (&httpTest{ + Name: "TerminaterDataStore not implemented", + Method: "DELETE", + URL: "foo", + ReqHeader: map[string]string{ + "Tus-Resumable": "1.0.0", + }, + Code: http.StatusMethodNotAllowed, + }).Run(handler, t) +} diff --git a/unrouted_handler.go b/unrouted_handler.go index 409fb36..920e30a 100644 --- a/unrouted_handler.go +++ b/unrouted_handler.go @@ -76,6 +76,7 @@ type UnroutedHandler struct { isBasePathAbs bool basePath string logger *log.Logger + extensions string // For each finished upload the corresponding info object will be sent using // this unbuffered channel. The NotifyCompleteUploads property in the Config @@ -108,6 +109,12 @@ func NewUnroutedHandler(config Config) (*UnroutedHandler, error) { base = "/" + base } + // Only promote extesions using the Tus-Extension header which are implemented + extensions := "creation,concatenation" + if _, ok := config.DataStore.(TerminaterDataStore); ok { + extensions += ",termination" + } + handler := &UnroutedHandler{ config: config, dataStore: config.DataStore, @@ -115,6 +122,7 @@ func NewUnroutedHandler(config Config) (*UnroutedHandler, error) { isBasePathAbs: uri.IsAbs(), CompleteUploads: make(chan FileInfo), logger: logger, + extensions: extensions, } return handler, nil @@ -167,7 +175,7 @@ func (handler *UnroutedHandler) Middleware(h http.Handler) http.Handler { } header.Set("Tus-Version", "1.0.0") - header.Set("Tus-Extension", "creation,concatenation,termination") + header.Set("Tus-Extension", handler.extensions) w.WriteHeader(http.StatusNoContent) return @@ -429,6 +437,13 @@ func (handler *UnroutedHandler) GetFile(w http.ResponseWriter, r *http.Request) // DelFile terminates an upload permanently. func (handler *UnroutedHandler) DelFile(w http.ResponseWriter, r *http.Request) { + // Abort the request handling if the required interface is not implemented + tstore, ok := handler.config.DataStore.(TerminaterDataStore) + if !ok { + handler.sendError(w, r, ErrNotImplemented) + return + } + id, err := extractIDFromPath(r.URL.Path) if err != nil { handler.sendError(w, r, err) @@ -444,7 +459,7 @@ func (handler *UnroutedHandler) DelFile(w http.ResponseWriter, r *http.Request) defer locker.UnlockUpload(id) } - err = handler.dataStore.Terminate(id) + err = tstore.Terminate(id) if err != nil { handler.sendError(w, r, err) return