From dc807782555848be3ea369144165853d968a0153 Mon Sep 17 00:00:00 2001 From: hexxa Date: Sat, 18 Sep 2021 23:07:54 +0800 Subject: [PATCH] fix(fs): fd must be closed after deleting, renaming --- src/fs/local/fs.go | 55 +++++++++++++++++++++++++++---- src/handlers/fileshdr/handlers.go | 1 + src/server/server_files_test.go | 33 +++++++++++++++++++ 3 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/fs/local/fs.go b/src/fs/local/fs.go index 819aed9..9a9864d 100644 --- a/src/fs/local/fs.go +++ b/src/fs/local/fs.go @@ -15,7 +15,7 @@ import ( "github.com/ihexxa/quickshare/src/idgen" ) -var ErrTooManyOpens = errors.New("too many opened files") +var ErrTooManyOpens = errors.New("too many opened files and failed to clean") type LocalFS struct { root string @@ -148,6 +148,35 @@ func (fs *LocalFS) Sync() error { return nil } +func (fs *LocalFS) CloseFDs(filePaths map[string]bool) error { + fs.opensMtx.Lock() + defer fs.opensMtx.Unlock() + return fs.closeFDs(filePaths) +} + +func (fs *LocalFS) closeFDs(filePaths map[string]bool) error { + var err error + for filePath := range filePaths { + info, ok := fs.opens[filePath] + if ok { + err = fs.closeInfo(filePath, info) + if err != nil { + return err + } + } + + info, ok = fs.readers[filePath] + if ok { + err = fs.closeInfo(filePath, info) + if err != nil { + return err + } + } + } + + return nil +} + func (fs *LocalFS) Close() error { fs.opensMtx.Lock() defer fs.opensMtx.Unlock() @@ -171,7 +200,7 @@ func (fs *LocalFS) Create(path string) error { if fs.isTooManyOpens() { closed, err := fs.closeOpens(false, false, map[string]bool{}) if err != nil || closed == 0 { - return fmt.Errorf("too many opens and fail to clean(%d): %w", closed, err) + return ErrTooManyOpens } } @@ -209,7 +238,12 @@ func (fs *LocalFS) Remove(entryPath string) error { if err != nil { return err } - return os.RemoveAll(fullpath) + + err = os.RemoveAll(fullpath) + if err != nil { + return err + } + return fs.CloseFDs(map[string]bool{fullpath: true}) } func (fs *LocalFS) Rename(oldpath, newpath string) error { @@ -231,7 +265,14 @@ func (fs *LocalFS) Rename(oldpath, newpath string) error { _, err = os.Stat(fullNewPath) if err != nil { if os.IsNotExist(err) { - return os.Rename(fullOldPath, fullNewPath) + err = os.Rename(fullOldPath, fullNewPath) + if err != nil { + return err + } + err = fs.CloseFDs(map[string]bool{fullOldPath: true}) + if err != nil { + return err + } } return err } @@ -253,7 +294,7 @@ func (fs *LocalFS) ReadAt(path string, b []byte, off int64) (int, error) { if fs.isTooManyOpens() { closed, err := fs.closeOpens(false, false, map[string]bool{}) if err != nil || closed == 0 { - return nil, fmt.Errorf("too many opens and fail to clean (%d): %w", closed, err) + return nil, ErrTooManyOpens } } @@ -304,7 +345,7 @@ func (fs *LocalFS) WriteAt(path string, b []byte, off int64) (int, error) { closed, err := fs.closeOpens(false, false, map[string]bool{}) if err != nil || closed == 0 { // TODO: return Eagain and make client retry later - return nil, fmt.Errorf("too many opens and fail to clean (%d): %w", closed, err) + return nil, ErrTooManyOpens } } @@ -367,7 +408,7 @@ func (fs *LocalFS) GetFileReader(path string) (fs.ReadCloseSeeker, uint64, error if fs.isTooManyOpens() { closed, err := fs.closeOpens(false, false, map[string]bool{}) if err != nil || closed == 0 { - return nil, 0, fmt.Errorf("too many opens and fail to clean (%d): %w", closed, err) + return nil, 0, ErrTooManyOpens } } diff --git a/src/handlers/fileshdr/handlers.go b/src/handlers/fileshdr/handlers.go index f67c8f1..d175de6 100644 --- a/src/handlers/fileshdr/handlers.go +++ b/src/handlers/fileshdr/handlers.go @@ -157,6 +157,7 @@ func (h *FileHandlers) Create(c *gin.Context) { err = h.deps.FS().Create(tmpFilePath) if err != nil { + fmt.Println("\nexist,", err, tmpFilePath) if os.IsExist(err) { // avoid adding file size more than once err = h.deps.Users().SetUsed(userIDInt, false, req.FileSize) diff --git a/src/server/server_files_test.go b/src/server/server_files_test.go index e3c3fe6..e65b6c7 100644 --- a/src/server/server_files_test.go +++ b/src/server/server_files_test.go @@ -155,6 +155,39 @@ func TestFileHandlers(t *testing.T) { } }) + t.Run("test: Upload-Delete-Upload: fd are closed correctly", func(t *testing.T) { + files := map[string]string{ + "qs/files/close_Fd/dup_file1": "12345678", + } + + for i := 0; i < 2; i++ { + for filePath, content := range files { + assertUploadOK(t, filePath, content, addr, token) + + // file operation(deleting tmp file) may be async + // so creating tmp file in the 2nd time may conflict with the first time if it is not deleted yet + // checking file until it is deleted + // TODO: use fs.Stat() to avoid flaky testing... + time.Sleep(1000) + + err = fs.Close() + if err != nil { + t.Fatal(err) + } + + res, _, errs := cl.Delete(filePath) + if len(errs) > 0 { + t.Fatal(errs) + } else if res.StatusCode != 200 { + t.Fatal(res.StatusCode) + } + + // tmpFile fd is closed, so follow must succeed + assertUploadOK(t, filePath, content, addr, token) + } + } + }) + t.Run("test files APIs: Create-UploadChunk-UploadStatus-Metadata-Delete", func(t *testing.T) { for filePath, content := range map[string]string{ "qs/files/path1/f1.md": "1111 1111 1111 1111",