From 4e151d380cb50f151f1be969a23c0b99897df131 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Tue, 26 Nov 2019 08:39:31 +0100 Subject: [PATCH] lib/versioner: Reduce surface area (#6186) * lib/versioner: Reduce surface area This is a refactor while I was anyway rooting around in the versioner. Instead of exporting every possible implementation and the factory and letting the caller do whatever, this now encapsulates all that and exposes a New() that takes a config.VersioningConfiguration. Given that and that we don't know (from the outside) how a versioner works or what state it keeps, we now just construct it once per folder and keep it around. Previously it was recreated for each restore request. * unparam * wip --- lib/config/folderconfiguration.go | 13 -------- lib/model/model.go | 54 +++++++++++++++++++++---------- lib/versioner/external.go | 14 ++++---- lib/versioner/external_test.go | 4 +-- lib/versioner/simple.go | 14 ++++---- lib/versioner/simple_test.go | 4 +-- lib/versioner/staggered.go | 34 +++++++++---------- lib/versioner/staggered_test.go | 4 +-- lib/versioner/trashcan.go | 20 ++++++------ lib/versioner/trashcan_test.go | 4 +-- lib/versioner/util.go | 11 ++++--- lib/versioner/versioner.go | 17 ++++++++-- 12 files changed, 107 insertions(+), 86 deletions(-) diff --git a/lib/config/folderconfiguration.go b/lib/config/folderconfiguration.go index 1b8e7f6b5..b59f4e5c4 100644 --- a/lib/config/folderconfiguration.go +++ b/lib/config/folderconfiguration.go @@ -18,7 +18,6 @@ import ( "github.com/syncthing/syncthing/lib/fs" "github.com/syncthing/syncthing/lib/protocol" "github.com/syncthing/syncthing/lib/util" - "github.com/syncthing/syncthing/lib/versioner" ) var ( @@ -105,18 +104,6 @@ func (f FolderConfiguration) Filesystem() fs.Filesystem { return f.cachedFilesystem } -func (f FolderConfiguration) Versioner() versioner.Versioner { - if f.Versioning.Type == "" { - return nil - } - versionerFactory, ok := versioner.Factories[f.Versioning.Type] - if !ok { - panic(fmt.Sprintf("Requested versioning type %q that does not exist", f.Versioning.Type)) - } - - return versionerFactory(f.ID, f.Filesystem(), f.Versioning.Params) -} - func (f FolderConfiguration) ModTimeWindow() time.Duration { return f.cachedModTimeWindow } diff --git a/lib/model/model.go b/lib/model/model.go index 36843fcb6..b26925a65 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -140,6 +140,7 @@ type model struct { folderRunners map[string]service // folder -> puller or scanner folderRunnerTokens map[string][]suture.ServiceToken // folder -> tokens for puller or scanner folderRestartMuts syncMutexMap // folder -> restart mutex + folderVersioners map[string]versioner.Versioner // folder -> versioner (may be nil) pmut sync.RWMutex // protects the below conn map[protocol.DeviceID]connections.Connection @@ -166,6 +167,7 @@ var ( errFolderNotRunning = errors.New("folder is not running") errFolderMissing = errors.New("no such folder") errNetworkNotAllowed = errors.New("network not allowed") + errNoVersioner = errors.New("folder has no versioner") // errors about why a connection is closed errIgnoredFolderRemoved = errors.New("folder no longer ignored") errReplacingConnection = errors.New("replacing connection") @@ -200,6 +202,7 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio folderIgnores: make(map[string]*ignore.Matcher), folderRunners: make(map[string]service), folderRunnerTokens: make(map[string][]suture.ServiceToken), + folderVersioners: make(map[string]versioner.Versioner), conn: make(map[protocol.DeviceID]connections.Connection), connRequestLimiters: make(map[protocol.DeviceID]*byteSemaphore), closed: make(map[protocol.DeviceID]chan struct{}), @@ -318,21 +321,29 @@ func (m *model) startFolderLocked(cfg config.FolderConfiguration) { } } - ver := cfg.Versioner() - if service, ok := ver.(suture.Service); ok { - // The versioner implements the suture.Service interface, so - // expects to be run in the background in addition to being called - // when files are going to be archived. - token := m.Add(service) - m.folderRunnerTokens[folder] = append(m.folderRunnerTokens[folder], token) - } - ffs := fset.MtimeFS() // These are our metadata files, and they should always be hidden. - ffs.Hide(config.DefaultMarkerName) - ffs.Hide(".stversions") - ffs.Hide(".stignore") + _ = ffs.Hide(config.DefaultMarkerName) + _ = ffs.Hide(".stversions") + _ = ffs.Hide(".stignore") + + var ver versioner.Versioner + if cfg.Versioning.Type != "" { + var err error + ver, err = versioner.New(ffs, cfg.Versioning) + if err != nil { + panic(fmt.Errorf("creating versioner: %v", err)) + } + if service, ok := ver.(suture.Service); ok { + // The versioner implements the suture.Service interface, so + // expects to be run in the background in addition to being called + // when files are going to be archived. + token := m.Add(service) + m.folderRunnerTokens[folder] = append(m.folderRunnerTokens[folder], token) + } + } + m.folderVersioners[folder] = ver ignores := m.folderIgnores[folder] @@ -459,6 +470,7 @@ func (m *model) removeFolderLocked(cfg config.FolderConfiguration) { delete(m.folderIgnores, cfg.ID) delete(m.folderRunners, cfg.ID) delete(m.folderRunnerTokens, cfg.ID) + delete(m.folderVersioners, cfg.ID) } func (m *model) restartFolder(from, to config.FolderConfiguration) { @@ -2444,14 +2456,14 @@ func (m *model) GlobalDirectoryTree(folder, prefix string, levels int, dirsonly } func (m *model) GetFolderVersions(folder string) (map[string][]versioner.FileVersion, error) { - fcfg, ok := m.cfg.Folder(folder) + m.fmut.RLock() + ver, ok := m.folderVersioners[folder] + m.fmut.RUnlock() if !ok { return nil, errFolderMissing } - - ver := fcfg.Versioner() if ver == nil { - return nil, errors.New("no versioner configured") + return nil, errNoVersioner } return ver.GetVersions() @@ -2463,7 +2475,15 @@ func (m *model) RestoreFolderVersions(folder string, versions map[string]time.Ti return nil, errFolderMissing } - ver := fcfg.Versioner() + m.fmut.RLock() + ver := m.folderVersioners[folder] + m.fmut.RUnlock() + if !ok { + return nil, errFolderMissing + } + if ver == nil { + return nil, errNoVersioner + } restoreErrors := make(map[string]string) diff --git a/lib/versioner/external.go b/lib/versioner/external.go index 9a7a687a9..a6c05fb64 100644 --- a/lib/versioner/external.go +++ b/lib/versioner/external.go @@ -21,22 +21,22 @@ import ( func init() { // Register the constructor for this type of versioner with the name "external" - Factories["external"] = NewExternal + factories["external"] = newExternal } -type External struct { +type external struct { command string filesystem fs.Filesystem } -func NewExternal(folderID string, filesystem fs.Filesystem, params map[string]string) Versioner { +func newExternal(filesystem fs.Filesystem, params map[string]string) Versioner { command := params["command"] if runtime.GOOS == "windows" { command = strings.Replace(command, `\`, `\\`, -1) } - s := External{ + s := external{ command: command, filesystem: filesystem, } @@ -47,7 +47,7 @@ func NewExternal(folderID string, filesystem fs.Filesystem, params map[string]st // Archive moves the named file away to a version archive. If this function // returns nil, the named file does not exist any more (has been archived). -func (v External) Archive(filePath string) error { +func (v external) Archive(filePath string) error { info, err := v.filesystem.Lstat(filePath) if fs.IsNotExist(err) { l.Debugln("not archiving nonexistent file", filePath) @@ -107,10 +107,10 @@ func (v External) Archive(filePath string) error { return errors.New("Versioner: file was not removed by external script") } -func (v External) GetVersions() (map[string][]FileVersion, error) { +func (v external) GetVersions() (map[string][]FileVersion, error) { return nil, ErrRestorationNotSupported } -func (v External) Restore(filePath string, versionTime time.Time) error { +func (v external) Restore(filePath string, versionTime time.Time) error { return ErrRestorationNotSupported } diff --git a/lib/versioner/external_test.go b/lib/versioner/external_test.go index 937cca32e..5979d5e76 100644 --- a/lib/versioner/external_test.go +++ b/lib/versioner/external_test.go @@ -29,7 +29,7 @@ func TestExternalNoCommand(t *testing.T) { // The versioner should fail due to missing command. - e := External{ + e := external{ filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "."), command: "nonexistent command", } @@ -62,7 +62,7 @@ func TestExternal(t *testing.T) { // The versioner should run successfully. - e := External{ + e := external{ filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "."), command: cmd, } diff --git a/lib/versioner/simple.go b/lib/versioner/simple.go index 9eef028ba..4e18cce83 100644 --- a/lib/versioner/simple.go +++ b/lib/versioner/simple.go @@ -15,22 +15,22 @@ import ( func init() { // Register the constructor for this type of versioner with the name "simple" - Factories["simple"] = NewSimple + factories["simple"] = newSimple } -type Simple struct { +type simple struct { keep int folderFs fs.Filesystem versionsFs fs.Filesystem } -func NewSimple(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner { +func newSimple(folderFs fs.Filesystem, params map[string]string) Versioner { keep, err := strconv.Atoi(params["keep"]) if err != nil { keep = 5 // A reasonable default } - s := Simple{ + s := simple{ keep: keep, folderFs: folderFs, versionsFs: fsFromParams(folderFs, params), @@ -42,7 +42,7 @@ func NewSimple(folderID string, folderFs fs.Filesystem, params map[string]string // Archive moves the named file away to a version archive. If this function // returns nil, the named file does not exist any more (has been archived). -func (v Simple) Archive(filePath string) error { +func (v simple) Archive(filePath string) error { err := archiveFile(v.folderFs, v.versionsFs, filePath, TagFilename) if err != nil { return err @@ -63,10 +63,10 @@ func (v Simple) Archive(filePath string) error { return nil } -func (v Simple) GetVersions() (map[string][]FileVersion, error) { +func (v simple) GetVersions() (map[string][]FileVersion, error) { return retrieveVersions(v.versionsFs) } -func (v Simple) Restore(filepath string, versionTime time.Time) error { +func (v simple) Restore(filepath string, versionTime time.Time) error { return restoreFile(v.versionsFs, v.folderFs, filepath, versionTime, TagFilename) } diff --git a/lib/versioner/simple_test.go b/lib/versioner/simple_test.go index e67e67316..768f4b5c9 100644 --- a/lib/versioner/simple_test.go +++ b/lib/versioner/simple_test.go @@ -41,7 +41,7 @@ func TestTaggedFilename(t *testing.T) { } // Test parser - tag := ExtractTag(tc[2]) + tag := extractTag(tc[2]) if tag != tc[1] { t.Errorf("%s != %s", tag, tc[1]) } @@ -61,7 +61,7 @@ func TestSimpleVersioningVersionCount(t *testing.T) { fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir) - v := NewSimple("", fs, map[string]string{"keep": "2"}) + v := newSimple(fs, map[string]string{"keep": "2"}) path := "test" diff --git a/lib/versioner/staggered.go b/lib/versioner/staggered.go index 69a0e88f1..dcbce433f 100644 --- a/lib/versioner/staggered.go +++ b/lib/versioner/staggered.go @@ -22,26 +22,26 @@ import ( func init() { // Register the constructor for this type of versioner with the name "staggered" - Factories["staggered"] = NewStaggered + factories["staggered"] = newStaggered } -type Interval struct { +type interval struct { step int64 end int64 } -type Staggered struct { +type staggered struct { suture.Service cleanInterval int64 folderFs fs.Filesystem versionsFs fs.Filesystem - interval [4]Interval + interval [4]interval mutex sync.Mutex testCleanDone chan struct{} } -func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner { +func newStaggered(folderFs fs.Filesystem, params map[string]string) Versioner { maxAge, err := strconv.ParseInt(params["maxAge"], 10, 0) if err != nil { maxAge = 31536000 // Default: ~1 year @@ -55,11 +55,11 @@ func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]str params["fsPath"] = params["versionsPath"] versionsFs := fsFromParams(folderFs, params) - s := &Staggered{ + s := &staggered{ cleanInterval: cleanInterval, folderFs: folderFs, versionsFs: versionsFs, - interval: [4]Interval{ + interval: [4]interval{ {30, 3600}, // first hour -> 30 sec between versions {3600, 86400}, // next day -> 1 h between versions {86400, 592000}, // next 30 days -> 1 day between versions @@ -73,7 +73,7 @@ func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]str return s } -func (v *Staggered) serve(ctx context.Context) { +func (v *staggered) serve(ctx context.Context) { v.clean() if v.testCleanDone != nil { close(v.testCleanDone) @@ -91,7 +91,7 @@ func (v *Staggered) serve(ctx context.Context) { } } -func (v *Staggered) clean() { +func (v *staggered) clean() { l.Debugln("Versioner clean: Waiting for lock on", v.versionsFs) v.mutex.Lock() defer v.mutex.Unlock() @@ -142,7 +142,7 @@ func (v *Staggered) clean() { l.Debugln("Cleaner: Finished cleaning", v.versionsFs) } -func (v *Staggered) expire(versions []string) { +func (v *staggered) expire(versions []string) { l.Debugln("Versioner: Expiring versions", versions) for _, file := range v.toRemove(versions, time.Now()) { if fi, err := v.versionsFs.Lstat(file); err != nil { @@ -159,7 +159,7 @@ func (v *Staggered) expire(versions []string) { } } -func (v *Staggered) toRemove(versions []string, now time.Time) []string { +func (v *staggered) toRemove(versions []string, now time.Time) []string { var prevAge int64 firstFile := true var remove []string @@ -168,7 +168,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string { sort.Strings(versions) for _, version := range versions { - versionTime, err := time.ParseInLocation(TimeFormat, ExtractTag(version), time.Local) + versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local) if err != nil { l.Debugf("Versioner: file name %q is invalid: %v", version, err) continue @@ -190,7 +190,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string { } // Find the interval the file fits in - var usedInterval Interval + var usedInterval interval for _, usedInterval = range v.interval { if age < usedInterval.end { break @@ -211,7 +211,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string { // Archive moves the named file away to a version archive. If this function // returns nil, the named file does not exist any more (has been archived). -func (v *Staggered) Archive(filePath string) error { +func (v *staggered) Archive(filePath string) error { l.Debugln("Waiting for lock on ", v.versionsFs) v.mutex.Lock() defer v.mutex.Unlock() @@ -225,14 +225,14 @@ func (v *Staggered) Archive(filePath string) error { return nil } -func (v *Staggered) GetVersions() (map[string][]FileVersion, error) { +func (v *staggered) GetVersions() (map[string][]FileVersion, error) { return retrieveVersions(v.versionsFs) } -func (v *Staggered) Restore(filepath string, versionTime time.Time) error { +func (v *staggered) Restore(filepath string, versionTime time.Time) error { return restoreFile(v.versionsFs, v.folderFs, filepath, versionTime, TagFilename) } -func (v *Staggered) String() string { +func (v *staggered) String() string { return fmt.Sprintf("Staggered/@%p", v) } diff --git a/lib/versioner/staggered_test.go b/lib/versioner/staggered_test.go index 9bc41f61e..a555fa659 100644 --- a/lib/versioner/staggered_test.go +++ b/lib/versioner/staggered_test.go @@ -57,9 +57,9 @@ func TestStaggeredVersioningVersionCount(t *testing.T) { } sort.Strings(delete) - v := NewStaggered("", fs.NewFilesystem(fs.FilesystemTypeFake, "testdata"), map[string]string{ + v := newStaggered(fs.NewFilesystem(fs.FilesystemTypeFake, "testdata"), map[string]string{ "maxAge": strconv.Itoa(365 * 86400), - }).(*Staggered) + }).(*staggered) rem := v.toRemove(versionsWithMtime, now) sort.Strings(rem) diff --git a/lib/versioner/trashcan.go b/lib/versioner/trashcan.go index bd408135e..70c68d38f 100644 --- a/lib/versioner/trashcan.go +++ b/lib/versioner/trashcan.go @@ -20,21 +20,21 @@ import ( func init() { // Register the constructor for this type of versioner - Factories["trashcan"] = NewTrashcan + factories["trashcan"] = newTrashcan } -type Trashcan struct { +type trashcan struct { suture.Service folderFs fs.Filesystem versionsFs fs.Filesystem cleanoutDays int } -func NewTrashcan(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner { +func newTrashcan(folderFs fs.Filesystem, params map[string]string) Versioner { cleanoutDays, _ := strconv.Atoi(params["cleanoutDays"]) // On error we default to 0, "do not clean out the trash can" - s := &Trashcan{ + s := &trashcan{ folderFs: folderFs, versionsFs: fsFromParams(folderFs, params), cleanoutDays: cleanoutDays, @@ -47,13 +47,13 @@ func NewTrashcan(folderID string, folderFs fs.Filesystem, params map[string]stri // Archive moves the named file away to a version archive. If this function // returns nil, the named file does not exist any more (has been archived). -func (t *Trashcan) Archive(filePath string) error { +func (t *trashcan) Archive(filePath string) error { return archiveFile(t.folderFs, t.versionsFs, filePath, func(name, tag string) string { return name }) } -func (t *Trashcan) serve(ctx context.Context) { +func (t *trashcan) serve(ctx context.Context) { l.Debugln(t, "starting") defer l.Debugln(t, "stopping") @@ -79,11 +79,11 @@ func (t *Trashcan) serve(ctx context.Context) { } } -func (t *Trashcan) String() string { +func (t *trashcan) String() string { return fmt.Sprintf("trashcan@%p", t) } -func (t *Trashcan) cleanoutArchive() error { +func (t *trashcan) cleanoutArchive() error { if _, err := t.versionsFs.Lstat("."); fs.IsNotExist(err) { return nil } @@ -121,11 +121,11 @@ func (t *Trashcan) cleanoutArchive() error { return nil } -func (t *Trashcan) GetVersions() (map[string][]FileVersion, error) { +func (t *trashcan) GetVersions() (map[string][]FileVersion, error) { return retrieveVersions(t.versionsFs) } -func (t *Trashcan) Restore(filepath string, versionTime time.Time) error { +func (t *trashcan) Restore(filepath string, versionTime time.Time) error { // If we have an untagged file A and want to restore it on top of existing file A, we can't first archive the // existing A as we'd overwrite the old A version, therefore when we archive existing file, we archive it with a // tag but when the restoration is finished, we rename it (untag it). This is only important if when restoring A, diff --git a/lib/versioner/trashcan_test.go b/lib/versioner/trashcan_test.go index 402146d45..e3d6b94aa 100644 --- a/lib/versioner/trashcan_test.go +++ b/lib/versioner/trashcan_test.go @@ -53,7 +53,7 @@ func TestTrashcanCleanout(t *testing.T) { } } - versioner := NewTrashcan("default", fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"), map[string]string{"cleanoutDays": "7"}).(*Trashcan) + versioner := newTrashcan(fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"), map[string]string{"cleanoutDays": "7"}).(*trashcan) if err := versioner.cleanoutArchive(); err != nil { t.Fatal(err) } @@ -95,7 +95,7 @@ func TestTrashcanArchiveRestoreSwitcharoo(t *testing.T) { writeFile(t, folderFs, "file", "A") - versioner := NewTrashcan("", folderFs, map[string]string{ + versioner := newTrashcan(folderFs, map[string]string{ "fsType": "basic", "fsPath": tmpDir2, }) diff --git a/lib/versioner/util.go b/lib/versioner/util.go index 1d4f3d1f5..d9da98b30 100644 --- a/lib/versioner/util.go +++ b/lib/versioner/util.go @@ -24,7 +24,7 @@ var errDirectory = fmt.Errorf("cannot restore on top of a directory") var errNotFound = fmt.Errorf("version not found") var errFileAlreadyExists = fmt.Errorf("file already exists") -// Inserts ~tag just before the extension of the filename. +// TagFilename inserts ~tag just before the extension of the filename. func TagFilename(name, tag string) string { dir, file := filepath.Dir(name), filepath.Base(name) ext := filepath.Ext(file) @@ -34,8 +34,8 @@ func TagFilename(name, tag string) string { var tagExp = regexp.MustCompile(`.*~([^~.]+)(?:\.[^.]+)?$`) -// Returns the tag from a filename, whether at the end or middle. -func ExtractTag(path string) string { +// extractTag returns the tag from a filename, whether at the end or middle. +func extractTag(path string) string { match := tagExp.FindStringSubmatch(path) // match is []string{"whole match", "submatch"} when successful @@ -45,9 +45,10 @@ func ExtractTag(path string) string { return match[1] } +// UntagFilename returns the filename without tag, and the extracted tag func UntagFilename(path string) (string, string) { ext := filepath.Ext(path) - versionTag := ExtractTag(path) + versionTag := extractTag(path) // Files tagged with old style tags cannot be untagged. if versionTag == "" { @@ -276,7 +277,7 @@ func findAllVersions(fs fs.Filesystem, filePath string) []string { file := filepath.Base(filePath) // Glob according to the new file~timestamp.ext pattern. - pattern := filepath.Join(inFolderPath, TagFilename(file, TimeGlob)) + pattern := filepath.Join(inFolderPath, TagFilename(file, timeGlob)) versions, err := fs.Glob(pattern) if err != nil { l.Warnln("globbing:", err, "for", pattern) diff --git a/lib/versioner/versioner.go b/lib/versioner/versioner.go index 8f9ee89b7..5cbeb6932 100644 --- a/lib/versioner/versioner.go +++ b/lib/versioner/versioner.go @@ -12,6 +12,7 @@ import ( "fmt" "time" + "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/fs" ) @@ -27,10 +28,22 @@ type FileVersion struct { Size int64 `json:"size"` } -var Factories = map[string]func(folderID string, filesystem fs.Filesystem, params map[string]string) Versioner{} +type factory func(filesystem fs.Filesystem, params map[string]string) Versioner + +var factories = make(map[string]factory) + var ErrRestorationNotSupported = fmt.Errorf("version restoration not supported with the current versioner") const ( TimeFormat = "20060102-150405" - TimeGlob = "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]" // glob pattern matching TimeFormat + timeGlob = "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]" // glob pattern matching TimeFormat ) + +func New(fs fs.Filesystem, cfg config.VersioningConfiguration) (Versioner, error) { + fac, ok := factories[cfg.Type] + if !ok { + return nil, fmt.Errorf("requested versioning type %q does not exist", cfg.Type) + } + + return fac(fs, cfg.Params), nil +}