Skip to content

Fix race in local storage #14888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Fix race in local storage #14888

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0f1621
Fix race in local storage
zeripath Mar 4, 2021
8d0a13e
Update modules/storage/local.go
zeripath Mar 4, 2021
84c90c5
Merge branch 'master' into fix-lfs-contentstore-races
6543 Mar 4, 2021
c9f4b3f
Merge branch 'master' into fix-lfs-contentstore-races
lafriks Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 38 additions & 10 deletions modules/storage/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package storage
import (
"context"
"io"
"io/ioutil"
"net/url"
"os"
"path/filepath"
Expand All @@ -24,13 +25,15 @@ const LocalStorageType Type = "local"

// LocalStorageConfig represents the configuration for a local storage
type LocalStorageConfig struct {
Path string `ini:"PATH"`
Path string `ini:"PATH"`
TemporaryPath string `ini:"TEMPORARY_PATH"`
}

// LocalStorage represents a local files storage
type LocalStorage struct {
ctx context.Context
dir string
ctx context.Context
dir string
tmpdir string
}

// NewLocalStorage returns a local files
Expand All @@ -46,9 +49,14 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error
return nil, err
}

if config.TemporaryPath == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put them to a system temp directory is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files may be large and the system temporary directory is not guaranteed to be able to cope.

The system tempdir may be on a different volume in Windows or may be on a different filesystem.

The only place we know for sure that LFS files will fit and should be renamed atomically is if they're in the LFS repository path.

The temporary directory is configurable - although I've not explicitly documented it here: set TEMPORARY_PATH for the storage

config.TemporaryPath = config.Path + "/tmp"
}

return &LocalStorage{
ctx: ctx,
dir: config.Path,
ctx: ctx,
dir: config.Path,
tmpdir: config.TemporaryPath,
}, nil
}

Expand All @@ -64,17 +72,37 @@ func (l *LocalStorage) Save(path string, r io.Reader) (int64, error) {
return 0, err
}

// always override
if err := util.Remove(p); err != nil {
// Create a temporary file to save to
if err := os.MkdirAll(l.tmpdir, os.ModePerm); err != nil {
return 0, err
}
tmp, err := ioutil.TempFile(l.tmpdir, "upload-*")
if err != nil {
return 0, err
}
tmpRemoved := false
defer func() {
if !tmpRemoved {
_ = util.Remove(tmp.Name())
}
}()

f, err := os.Create(p)
n, err := io.Copy(tmp, r)
if err != nil {
return 0, err
}
defer f.Close()
return io.Copy(f, r)

if err := tmp.Close(); err != nil {
return 0, err
}

if err := os.Rename(tmp.Name(), p); err != nil {
return 0, err
}

tmpRemoved = true

return n, nil
}

// Stat returns the info of the file
Expand Down