Permalink
Browse files

fs: Protect filesystem access with sync.RWMutex.

Make service methods safe for concurrent use by protecting filesystem
access with a reader/writer mutex.

Remove obsolete TODO comments; they no longer apply. The "Doing this
here before committing" text was referring to making a s.users.Get call
that could've failed, hence it was being done committing to storage.
By now, only UserSpecs are used, which cannot produce errors, but
the local variables are still created in advance so they can be reused.

Similar to shurcooL/notifications@627ab5a
and shurcooL/reactions@dc7624d.
  • Loading branch information...
dmitshur committed Oct 7, 2018
1 parent c5ffda8 commit e7213ea3c1d80222b513e8b6e6574c912ff1e39a
Showing with 53 additions and 24 deletions.
  1. +5 −2 fs/copier.go
  2. +2 −2 fs/events.go
  3. +40 −14 fs/fs.go
  4. +4 −4 fs/notifications.go
  5. +1 −1 fs/schema.go
  6. +1 −1 fs/users.go
View
@@ -7,9 +7,12 @@ import (
"github.com/shurcooL/issues"
)
var _ issues.CopierFrom = service{}
var _ issues.CopierFrom = &service{}
func (s *service) CopyFrom(ctx context.Context, src issues.Service, repo issues.RepoSpec) error {
s.fsMu.Lock()
defer s.fsMu.Unlock()
func (s service) CopyFrom(ctx context.Context, src issues.Service, repo issues.RepoSpec) error {
if err := s.createNamespace(ctx, repo); err != nil {
return err
}
View
@@ -10,7 +10,7 @@ import (
"github.com/shurcooL/users"
)
func (s service) logIssue(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, issue issue, actor users.User, action string, time time.Time) error {
func (s *service) logIssue(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, issue issue, actor users.User, action string, time time.Time) error {
if s.events == nil {
return nil
}
@@ -29,7 +29,7 @@ func (s service) logIssue(ctx context.Context, repo issues.RepoSpec, issueID uin
return s.events.Log(ctx, event)
}
func (s service) logIssueComment(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, actor users.User, time time.Time, body string) error {
func (s *service) logIssueComment(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, actor users.User, time time.Time, body string) error {
if s.events == nil {
return nil
}
View
@@ -8,6 +8,7 @@ import (
"log"
"os"
"strconv"
"sync"
"time"
"github.com/shurcooL/events"
@@ -22,7 +23,7 @@ import (
// It uses notifications service, if not nil.
// It uses events service, if not nil.
func NewService(root webdav.FileSystem, notifications notifications.ExternalService, events events.ExternalService, users users.Service) (issues.Service, error) {
return service{
return &service{
fs: root,
notifications: notifications,
events: events,
@@ -31,7 +32,8 @@ func NewService(root webdav.FileSystem, notifications notifications.ExternalServ
}
type service struct {
fs webdav.FileSystem
fsMu sync.RWMutex
fs webdav.FileSystem
// notifications may be nil if there's no notifications service.
notifications notifications.ExternalService
@@ -41,11 +43,14 @@ type service struct {
users users.Service
}
func (s service) List(ctx context.Context, repo issues.RepoSpec, opt issues.IssueListOptions) ([]issues.Issue, error) {
func (s *service) List(ctx context.Context, repo issues.RepoSpec, opt issues.IssueListOptions) ([]issues.Issue, error) {
if opt.State != issues.StateFilter(issues.OpenState) && opt.State != issues.StateFilter(issues.ClosedState) && opt.State != issues.AllStates {
return nil, fmt.Errorf("invalid issues.IssueListOptions.State value: %q", opt.State) // TODO: Map to 400 Bad Request HTTP error.
}
s.fsMu.RLock()
defer s.fsMu.RUnlock()
var is []issues.Issue
dirs, err := readDirIDs(ctx, s.fs, issuesDir(repo))
@@ -98,11 +103,14 @@ func (s service) List(ctx context.Context, repo issues.RepoSpec, opt issues.Issu
return is, nil
}
func (s service) Count(ctx context.Context, repo issues.RepoSpec, opt issues.IssueListOptions) (uint64, error) {
func (s *service) Count(ctx context.Context, repo issues.RepoSpec, opt issues.IssueListOptions) (uint64, error) {
if opt.State != issues.StateFilter(issues.OpenState) && opt.State != issues.StateFilter(issues.ClosedState) && opt.State != issues.AllStates {
return 0, fmt.Errorf("invalid issues.IssueListOptions.State value: %q", opt.State) // TODO: Map to 400 Bad Request HTTP error.
}
s.fsMu.RLock()
defer s.fsMu.RUnlock()
var count uint64
dirs, err := readDirIDs(ctx, s.fs, issuesDir(repo))
@@ -132,12 +140,15 @@ func (s service) Count(ctx context.Context, repo issues.RepoSpec, opt issues.Iss
return count, nil
}
func (s service) Get(ctx context.Context, repo issues.RepoSpec, id uint64) (issues.Issue, error) {
func (s *service) Get(ctx context.Context, repo issues.RepoSpec, id uint64) (issues.Issue, error) {
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
return issues.Issue{}, err
}
s.fsMu.RLock()
defer s.fsMu.RUnlock()
var issue issue
err = jsonDecodeFile(ctx, s.fs, issueCommentPath(repo, id, 0), &issue)
if err != nil {
@@ -180,12 +191,15 @@ func (s service) Get(ctx context.Context, repo issues.RepoSpec, id uint64) (issu
}, nil
}
func (s service) ListComments(ctx context.Context, repo issues.RepoSpec, id uint64, opt *issues.ListOptions) ([]issues.Comment, error) {
func (s *service) ListComments(ctx context.Context, repo issues.RepoSpec, id uint64, opt *issues.ListOptions) ([]issues.Comment, error) {
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
return nil, err
}
s.fsMu.RLock()
defer s.fsMu.RUnlock()
var comments []issues.Comment
fis, err := readDirIDs(ctx, s.fs, issueDir(repo, id))
@@ -233,7 +247,10 @@ func (s service) ListComments(ctx context.Context, repo issues.RepoSpec, id uint
return comments, nil
}
func (s service) ListEvents(ctx context.Context, repo issues.RepoSpec, id uint64, opt *issues.ListOptions) ([]issues.Event, error) {
func (s *service) ListEvents(ctx context.Context, repo issues.RepoSpec, id uint64, opt *issues.ListOptions) ([]issues.Event, error) {
s.fsMu.RLock()
defer s.fsMu.RUnlock()
var events []issues.Event
fis, err := readDirIDs(ctx, s.fs, issueEventsDir(repo, id))
@@ -269,7 +286,7 @@ func (s service) ListEvents(ctx context.Context, repo issues.RepoSpec, id uint64
return events, nil
}
func (s service) CreateComment(ctx context.Context, repo issues.RepoSpec, id uint64, c issues.Comment) (issues.Comment, error) {
func (s *service) CreateComment(ctx context.Context, repo issues.RepoSpec, id uint64, c issues.Comment) (issues.Comment, error) {
// CreateComment operation requires an authenticated user with read access.
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
@@ -283,6 +300,9 @@ func (s service) CreateComment(ctx context.Context, repo issues.RepoSpec, id uin
return issues.Comment{}, err
}
s.fsMu.Lock()
defer s.fsMu.Unlock()
comment := comment{
Author: fromUserSpec(currentUser.UserSpec),
CreatedAt: time.Now().UTC(),
@@ -330,7 +350,7 @@ func (s service) CreateComment(ctx context.Context, repo issues.RepoSpec, id uin
}, nil
}
func (s service) Create(ctx context.Context, repo issues.RepoSpec, i issues.Issue) (issues.Issue, error) {
func (s *service) Create(ctx context.Context, repo issues.RepoSpec, i issues.Issue) (issues.Issue, error) {
// Create operation requires an authenticated user with read access.
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
@@ -344,6 +364,9 @@ func (s service) Create(ctx context.Context, repo issues.RepoSpec, i issues.Issu
return issues.Issue{}, err
}
s.fsMu.Lock()
defer s.fsMu.Unlock()
if err := s.createNamespace(ctx, repo); err != nil {
return issues.Issue{}, err
}
@@ -448,7 +471,7 @@ func canReact(currentUser users.UserSpec) error {
return nil
}
func (s service) Edit(ctx context.Context, repo issues.RepoSpec, id uint64, ir issues.IssueRequest) (issues.Issue, []issues.Event, error) {
func (s *service) Edit(ctx context.Context, repo issues.RepoSpec, id uint64, ir issues.IssueRequest) (issues.Issue, []issues.Event, error) {
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
return issues.Issue{}, nil, err
@@ -461,6 +484,9 @@ func (s service) Edit(ctx context.Context, repo issues.RepoSpec, id uint64, ir i
return issues.Issue{}, nil, err
}
s.fsMu.Lock()
defer s.fsMu.Unlock()
// Get from storage.
var issue issue
err = jsonDecodeFile(ctx, s.fs, issueCommentPath(repo, id, 0), &issue)
@@ -473,7 +499,6 @@ func (s service) Edit(ctx context.Context, repo issues.RepoSpec, id uint64, ir i
return issues.Issue{}, nil, err
}
// TODO: Doing this here before committing in case it fails; think about factoring this out into a user service that augments...
author := issue.Author.UserSpec()
actor := currentUser.UserSpec
@@ -569,7 +594,7 @@ func (s service) Edit(ctx context.Context, repo issues.RepoSpec, id uint64, ir i
}, events, nil
}
func (s service) EditComment(ctx context.Context, repo issues.RepoSpec, id uint64, cr issues.CommentRequest) (issues.Comment, error) {
func (s *service) EditComment(ctx context.Context, repo issues.RepoSpec, id uint64, cr issues.CommentRequest) (issues.Comment, error) {
currentUser, err := s.users.GetAuthenticated(ctx)
if err != nil {
return issues.Comment{}, err
@@ -583,6 +608,9 @@ func (s service) EditComment(ctx context.Context, repo issues.RepoSpec, id uint6
return issues.Comment{}, err
}
s.fsMu.Lock()
defer s.fsMu.Unlock()
// TODO: Merge these 2 cases (first comment aka issue vs reply comments) into one.
if cr.ID == 0 {
// Get from storage.
@@ -604,7 +632,6 @@ func (s service) EditComment(ctx context.Context, repo issues.RepoSpec, id uint6
}
}
// TODO: Doing this here before committing in case it fails; think about factoring this out into a user service that augments...
author := issue.Author.UserSpec()
actor := currentUser.UserSpec
editedAt := time.Now().UTC()
@@ -689,7 +716,6 @@ func (s service) EditComment(ctx context.Context, repo issues.RepoSpec, id uint6
}
}
// TODO: Doing this here before committing in case it fails; think about factoring this out into a user service that augments...
author := comment.Author.UserSpec()
actor := currentUser.UserSpec
editedAt := time.Now().UTC()
View
@@ -15,10 +15,10 @@ import (
const threadType = "issues"
// ThreadType returns the notifications thread type for this service.
func (service) ThreadType(issues.RepoSpec) string { return threadType }
func (*service) ThreadType(issues.RepoSpec) string { return threadType }
// subscribe subscribes user and anyone mentioned in body to the issue.
func (s service) subscribe(ctx context.Context, repo issues.RepoSpec, issueID uint64, user users.UserSpec, body string) error {
func (s *service) subscribe(ctx context.Context, repo issues.RepoSpec, issueID uint64, user users.UserSpec, body string) error {
if s.notifications == nil {
return nil
}
@@ -36,7 +36,7 @@ func (s service) subscribe(ctx context.Context, repo issues.RepoSpec, issueID ui
}
// markRead marks the specified issue as read for current user.
func (s service) markRead(ctx context.Context, repo issues.RepoSpec, issueID uint64) error {
func (s *service) markRead(ctx context.Context, repo issues.RepoSpec, issueID uint64) error {
if s.notifications == nil {
return nil
}
@@ -45,7 +45,7 @@ func (s service) markRead(ctx context.Context, repo issues.RepoSpec, issueID uin
}
// notify notifies all subscribed users of an update that shows up in their Notification Center.
func (s service) notify(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, actor users.UserSpec, time time.Time) error {
func (s *service) notify(ctx context.Context, repo issues.RepoSpec, issueID uint64, fragment string, actor users.UserSpec, time time.Time) error {
if s.notifications == nil {
return nil
}
View
@@ -210,7 +210,7 @@ func (c commit) Commit() issues.Commit {
// ├── 0
// └── events
func (s service) createNamespace(ctx context.Context, repo issues.RepoSpec) error {
func (s *service) createNamespace(ctx context.Context, repo issues.RepoSpec) error {
if path.Clean("/"+repo.URI) != "/"+repo.URI {
return fmt.Errorf("invalid repo.URI (not clean): %q", repo.URI)
}
View
@@ -7,7 +7,7 @@ import (
"github.com/shurcooL/users"
)
func (s service) user(ctx context.Context, user users.UserSpec) users.User {
func (s *service) user(ctx context.Context, user users.UserSpec) users.User {
u, err := s.users.Get(ctx, user)
if err != nil {
return users.User{

0 comments on commit e7213ea

Please sign in to comment.