From 6eeea854be6bea74320bf47c5e666a56a924b05f Mon Sep 17 00:00:00 2001 From: Conrad Hoffmann Date: Thu, 7 Nov 2024 12:14:45 +0100 Subject: [PATCH] storage/filesystem: add R/W locking This commit adds read/write locking for individual files, so that concurrent requests (e.g. to read and write the same file) cannot interfere with one another. The locking is not very fine-grained at the moment, and can probably be improved upon. But it does ensure consistency. --- storage/filesystem.go | 35 +++++++++++++++++++++++++++++++++++ storage/filesystem_caldav.go | 27 ++++++++++++++++++++++++++- storage/filesystem_carddav.go | 28 +++++++++++++++++++++++++++- 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/storage/filesystem.go b/storage/filesystem.go index afc058b..c6f3887 100644 --- a/storage/filesystem.go +++ b/storage/filesystem.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" "github.com/rs/zerolog/log" @@ -20,9 +21,13 @@ import ( type filesystemBackend struct { webdav.UserPrincipalBackend + path string caldavPrefix string carddavPrefix string + + // maps file path to *sync.RWMutex + locks sync.Map } var ( @@ -47,6 +52,36 @@ func NewFilesystem(fsPath, caldavPrefix, carddavPrefix string, userPrincipalBack return backend, backend, nil } +func (b *filesystemBackend) RLock(filename string) { + lock_, _ := b.locks.LoadOrStore(filename, &sync.RWMutex{}) + lock := lock_.(*sync.RWMutex) + lock.RLock() +} + +func (b *filesystemBackend) RUnlock(filename string) { + lock_, ok := b.locks.Load(filename) + if !ok { + panic("attempt to unlock non-existing lock") + } + lock := lock_.(*sync.RWMutex) + lock.RUnlock() +} + +func (b *filesystemBackend) Lock(filename string) { + lock_, _ := b.locks.LoadOrStore(filename, &sync.RWMutex{}) + lock := lock_.(*sync.RWMutex) + lock.Lock() +} + +func (b *filesystemBackend) Unlock(filename string) { + lock_, ok := b.locks.Load(filename) + if !ok { + panic("attempt to unlock non-existing lock") + } + lock := lock_.(*sync.RWMutex) + lock.Unlock() +} + func ensureLocalDir(path string) error { if _, err := os.Stat(path); os.IsNotExist(err) { err = os.MkdirAll(path, 0755) diff --git a/storage/filesystem_caldav.go b/storage/filesystem_caldav.go index 1162fd0..a495ba5 100644 --- a/storage/filesystem_caldav.go +++ b/storage/filesystem_caldav.go @@ -85,6 +85,9 @@ func (b *filesystemBackend) loadAllCalendarObjects(ctx context.Context, urlPath return nil } + b.RLock(filename) + defer b.RUnlock(filename) + cal, err := calendarFromFile(filename, propFilter) if err != nil { fmt.Printf("load calendar error for %s: %v\n", filename, err) @@ -138,7 +141,12 @@ func (b *filesystemBackend) createDefaultCalendar(ctx context.Context) (*caldav. if err != nil { return nil, fmt.Errorf("error creating default calendar: %s", err.Error()) } - err = os.WriteFile(path.Join(localPath, calendarFileName), blob, 0644) + + filename := path.Join(localPath, calendarFileName) + b.Lock(filename) + defer b.Unlock(filename) + + err = os.WriteFile(filename, blob, 0644) if err != nil { return nil, fmt.Errorf("error writing default calendar: %s", err.Error()) } @@ -167,6 +175,10 @@ func (b *filesystemBackend) ListCalendars(ctx context.Context) ([]caldav.Calenda } calPath := path.Join(filename, calendarFileName) + + b.RLock(calPath) + defer b.RUnlock(calPath) + data, err := os.ReadFile(calPath) if err != nil { if os.IsNotExist(err) { @@ -209,6 +221,9 @@ func (b *filesystemBackend) GetCalendar(ctx context.Context, urlPath string) (*c log.Debug().Str("path", localPath).Msg("loading calendar") + b.RLock(localPath) + defer b.RUnlock(localPath) + data, err := os.ReadFile(localPath) if err != nil { if os.IsNotExist(err) { @@ -237,6 +252,9 @@ func (b *filesystemBackend) GetCalendarObject(ctx context.Context, objPath strin return nil, err } + b.RLock(localPath) + defer b.RUnlock(localPath) + info, err := os.Stat(localPath) if err != nil { if errors.Is(err, fs.ErrNotExist) { @@ -320,6 +338,9 @@ func (b *filesystemBackend) PutCalendarObject(ctx context.Context, objPath strin return nil, err } + b.Lock(localPath) + defer b.Unlock(localPath) + flags := os.O_RDWR | os.O_CREATE | os.O_TRUNC // TODO handle IfNoneMatch == ETag if opts.IfNoneMatch.IsWildcard() { @@ -384,6 +405,10 @@ func (b *filesystemBackend) DeleteCalendarObject(ctx context.Context, path strin if err != nil { return err } + + b.Lock(localPath) + defer b.Unlock(localPath) + err = os.Remove(localPath) if err != nil { return err diff --git a/storage/filesystem_carddav.go b/storage/filesystem_carddav.go index 764dc10..01f0c89 100644 --- a/storage/filesystem_carddav.go +++ b/storage/filesystem_carddav.go @@ -69,6 +69,7 @@ func vcardPropFilter(card vcard.Card, props []string) vcard.Card { } func vcardFromFile(path string, propFilter []string) (vcard.Card, error) { + f, err := os.Open(path) if err != nil { return nil, err @@ -103,6 +104,10 @@ func (b *filesystemBackend) loadAllAddressObjects(ctx context.Context, urlPath s return nil } + // TODO use single file read for data & etag + b.RLock(filename) + defer b.RUnlock(filename) + card, err := vcardFromFile(filename, propFilter) if err != nil { return err @@ -141,7 +146,12 @@ func (b *filesystemBackend) writeAddressBook(ctx context.Context, ab *carddav.Ad if err != nil { return err } - return os.WriteFile(path.Join(localPath, addressBookFileName), blob, 0644) + + filename := path.Join(localPath, addressBookFileName) + b.Lock(filename) + defer b.Unlock(filename) + + err = os.WriteFile(filename, blob, 0644) if err != nil { return fmt.Errorf("error writing address book: %s", err.Error()) } @@ -208,6 +218,9 @@ func (b *filesystemBackend) ListAddressBooks(ctx context.Context) ([]carddav.Add } abPath := path.Join(filename, addressBookFileName) + b.RLock(abPath) + defer b.RUnlock(abPath) + data, err := os.ReadFile(abPath) if err != nil { if os.IsNotExist(err) { @@ -250,6 +263,9 @@ func (b *filesystemBackend) GetAddressBook(ctx context.Context, urlPath string) log.Debug().Str("path", localPath).Msg("loading addressbook") + b.RLock(localPath) + defer b.RUnlock(localPath) + data, err := os.ReadFile(localPath) if err != nil { if os.IsNotExist(err) { @@ -303,6 +319,9 @@ func (b *filesystemBackend) GetAddressObject(ctx context.Context, objPath string return nil, err } + b.RLock(localPath) + defer b.RUnlock(localPath) + info, err := os.Stat(localPath) if err != nil { if errors.Is(err, fs.ErrNotExist) { @@ -381,6 +400,9 @@ func (b *filesystemBackend) PutAddressObject(ctx context.Context, objPath string return nil, err } + b.Lock(localPath) + defer b.Unlock(localPath) + flags := os.O_RDWR | os.O_CREATE | os.O_TRUNC // TODO handle IfNoneMatch == ETag if opts.IfNoneMatch.IsWildcard() { @@ -444,6 +466,10 @@ func (b *filesystemBackend) DeleteAddressObject(ctx context.Context, path string if err != nil { return err } + + b.Lock(localPath) + defer b.Unlock(localPath) + err = os.Remove(localPath) if err != nil { return err