From 5f02f5390ee7117edb85dfa7aed72329bcdf1101 Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:04:04 +0200 Subject: [PATCH 1/8] perf(ftp): use MLST or LIST first to determine if path exists over ftp --- cli/disk/ftp.go | 113 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 33 deletions(-) diff --git a/cli/disk/ftp.go b/cli/disk/ftp.go index 1900383..e6a16ec 100644 --- a/cli/disk/ftp.go +++ b/cli/disk/ftp.go @@ -3,12 +3,14 @@ package disk import ( "bytes" "context" + "errors" "fmt" "io" "log" "log/slog" + "net/textproto" "net/url" - "path/filepath" + "path" "strings" "time" @@ -91,57 +93,102 @@ func testFTP(u *url.URL, options ...ftp.DialOption) (*ftp.ServerConn, bool, erro return c, false, nil } -func (l *ftpDisk) Exists(path string) (bool, error) { - res, err := l.acquire() - if err != nil { - return false, err - } +func (l *ftpDisk) existsWithLock(res *puddle.Resource[*ftp.ServerConn], p string) (bool, error) { + slog.Debug("checking if file exists", slog.String("path", clean(p)), slog.String("schema", "ftp")) - defer res.Release() + var protocolError *textproto.Error - slog.Debug("checking if file exists", slog.String("path", clean(path)), slog.String("schema", "ftp")) + _, err := res.Value().GetEntry(clean(p)) + if err == nil { + return true, nil + } - split := strings.Split(clean(path)[1:], "/") - for _, s := range split[:len(split)-1] { - dir, err := l.readDirLock(res, "") - if err != nil { - return false, err + if errors.As(err, &protocolError) { + switch protocolError.Code { + case ftp.StatusFileUnavailable: + return false, nil + case ftp.StatusNotImplemented: + // GetEntry uses MLST, which might not be supported by the server. + // Even though in this case the error is not coming from the server, + // the ftp library still returns it as a protocol error. + default: + // We won't handle any other kind of error, such as + // * temporary errors (4xx) - should be retried after a while, so we won't deal with the delay + // * connection errors (x2x) - can't really do anything about them + // * authentication errors (x3x) - can't do anything about them + return false, fmt.Errorf("failed to get path info: %w", err) } + } else { + // This is a non-protocol error, so we can't be sure what it means. + return false, fmt.Errorf("failed to get path info: %w", err) + } - currentDir, _ := res.Value().CurrentDir() + // In case MLST is not supported, we can try to LIST the target path. + // We can be sure that List() will actually execute LIST and not MLSD, + // since MLST was not supported in the previous step. + entries, err := res.Value().List(clean(p)) + if err == nil { + if len(entries) > 0 { + // Some server implementations return an empty list for a nonexistent path, + // so we cannot be sure that no error means a directory exists unless it also contains some items. + // For files, when they exist, they will be listed as a single entry. + // TODO: so far the servers (just one) this was happening on also listed . and .. for valid dirs, because it was using `LIST -a`. Is that behaviour consistent that we can rely on it? + return true, nil + } + } else { + if errors.As(err, &protocolError) { + switch protocolError.Code { + case ftp.StatusFileUnavailable: + return false, nil + default: + // We won't handle any other kind of error, see above. + return false, fmt.Errorf("failed to list path: %w", err) + } + } + // This is a non-protocol error, see above. + return false, fmt.Errorf("failed to list path: %w", err) + } - foundDir := false + // If we got here, either the path is an empty directory, + // or it does not exist and the server is a weird implementation. + + // List the parent directory to determine if the path exists + dir, err := l.readDirLock(res, path.Dir(clean(p))) + if err == nil { + found := false for _, entry := range dir { - if entry.IsDir() && entry.Name() == s { - foundDir = true + if entry.Name() == path.Base(clean(p)) { + found = true break } } - if !foundDir { - return false, nil - } + return found, nil + } - slog.Debug("entering directory", slog.String("dir", s), slog.String("cwd", currentDir), slog.String("schema", "ftp")) - if err := res.Value().ChangeDir(s); err != nil { - return false, fmt.Errorf("failed to enter directory: %w", err) + if errors.As(err, &protocolError) { + switch protocolError.Code { + case ftp.StatusFileUnavailable: + return false, nil + default: + // We won't handle any other kind of error, see above. + return false, fmt.Errorf("failed to list path: %w", err) } } - dir, err := l.readDirLock(res, "") + // This is a non-protocol error, see above. + return false, fmt.Errorf("failed to list path: %w", err) +} + +func (l *ftpDisk) Exists(p string) (bool, error) { + res, err := l.acquire() if err != nil { - return false, fmt.Errorf("failed listing directory: %w", err) + return false, err } - found := false - for _, entry := range dir { - if entry.Name() == clean(filepath.Base(path)) { - found = true - break - } - } + defer res.Release() - return found, nil + return l.existsWithLock(res, p) } func (l *ftpDisk) Read(path string) ([]byte, error) { From d6b29067e8fe5359055017f9e371ec5230752a60 Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:05:37 +0200 Subject: [PATCH 2/8] perf(ftp): optimistically check directories from target path up when creating directory --- cli/disk/ftp.go | 49 +++++++++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/cli/disk/ftp.go b/cli/disk/ftp.go index e6a16ec..318db1c 100644 --- a/cli/disk/ftp.go +++ b/cli/disk/ftp.go @@ -250,7 +250,7 @@ func (l *ftpDisk) Remove(path string) error { return nil } -func (l *ftpDisk) MkDir(path string) error { +func (l *ftpDisk) MkDir(p string) error { res, err := l.acquire() if err != nil { return err @@ -258,34 +258,47 @@ func (l *ftpDisk) MkDir(path string) error { defer res.Release() - split := strings.Split(clean(path)[1:], "/") - for _, s := range split { - dir, err := l.readDirLock(res, "") + lastExistingDir := clean(p) + for lastExistingDir != "/" && lastExistingDir != "." { + foundDir, err := l.existsWithLock(res, lastExistingDir) if err != nil { return err } - currentDir, _ := res.Value().CurrentDir() - - foundDir := false - for _, entry := range dir { - if entry.IsDir() && entry.Name() == s { - foundDir = true - break - } + if foundDir { + break } - if !foundDir { - slog.Debug("making directory", slog.String("dir", s), slog.String("cwd", currentDir), slog.String("schema", "ftp")) - if err := res.Value().MakeDir(s); err != nil { - return fmt.Errorf("failed to make directory: %w", err) - } + lastExistingDir = path.Dir(lastExistingDir) + } + + remainingDirs := clean(p) + + if lastExistingDir != "/" && lastExistingDir != "." { + remainingDirs = strings.TrimPrefix(remainingDirs, lastExistingDir) + } + + if len(remainingDirs) == 0 { + // Already exists + return nil + } + + if err := res.Value().ChangeDir(lastExistingDir); err != nil { + return fmt.Errorf("failed to enter directory: %w", err) + } + + split := strings.Split(clean(remainingDirs)[1:], "/") + for _, s := range split { + slog.Debug("making directory", slog.String("dir", s), slog.String("cwd", lastExistingDir), slog.String("schema", "ftp")) + if err := res.Value().MakeDir(s); err != nil { + return fmt.Errorf("failed to make directory: %w", err) } - slog.Debug("entering directory", slog.String("dir", s), slog.String("cwd", currentDir), slog.String("schema", "ftp")) + slog.Debug("entering directory", slog.String("dir", s), slog.String("cwd", lastExistingDir), slog.String("schema", "ftp")) if err := res.Value().ChangeDir(s); err != nil { return fmt.Errorf("failed to enter directory: %w", err) } + lastExistingDir = path.Join(lastExistingDir, s) } return nil From 30f31d69e8ee0e9c0d3c07c9ae22aa745eb8ac37 Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:52:40 +0200 Subject: [PATCH 3/8] fix(ftp): skip . and .. in ReadDir --- cli/disk/ftp.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cli/disk/ftp.go b/cli/disk/ftp.go index 318db1c..e8884c2 100644 --- a/cli/disk/ftp.go +++ b/cli/disk/ftp.go @@ -11,6 +11,7 @@ import ( "net/textproto" "net/url" "path" + "slices" "strings" "time" @@ -312,7 +313,14 @@ func (l *ftpDisk) ReadDir(path string) ([]Entry, error) { defer res.Release() - return l.readDirLock(res, path) + entries, err := l.readDirLock(res, path) + if err != nil { + return nil, err + } + entries = slices.DeleteFunc(entries, func(i Entry) bool { + return i.Name() == "." || i.Name() == ".." + }) + return entries, nil } func (l *ftpDisk) readDirLock(res *puddle.Resource[*ftp.ServerConn], path string) ([]Entry, error) { From dc7c38439d34b91936445948b5ce8c421dea021e Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:53:20 +0200 Subject: [PATCH 4/8] perf(remote): parallelize old mod removal --- cli/installations.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/cli/installations.go b/cli/installations.go index dc5005b..b70792d 100644 --- a/cli/installations.go +++ b/cli/installations.go @@ -421,25 +421,35 @@ func (i *Installation) Install(ctx *GlobalContext, updates chan<- InstallUpdate) return fmt.Errorf("failed to read mods directory: %w", err) } + var deleteWait errgroup.Group for _, entry := range dir { if entry.IsDir() { if _, ok := lockfile.Mods[entry.Name()]; !ok { - modDir := filepath.Join(modsDirectory, entry.Name()) - exists, err := d.Exists(filepath.Join(modDir, ".smm")) - if err != nil { - return err - } + modName := entry.Name() + modDir := filepath.Join(modsDirectory, modName) + deleteWait.Go(func() error { + exists, err := d.Exists(filepath.Join(modDir, ".smm")) + if err != nil { + return err + } - if exists { - slog.Info("deleting mod", slog.String("mod_reference", entry.Name())) - if err := d.Remove(modDir); err != nil { - return fmt.Errorf("failed to delete mod directory: %w", err) + if exists { + slog.Info("deleting mod", slog.String("mod_reference", modName)) + if err := d.Remove(modDir); err != nil { + return fmt.Errorf("failed to delete mod directory: %w", err) + } } - } + + return nil + }) } } } + if err := deleteWait.Wait(); err != nil { + return fmt.Errorf("failed to remove old mods: %w", err) + } + slog.Info("starting installation", slog.Int("concurrency", viper.GetInt("concurrent-downloads")), slog.String("path", i.Path)) errg := errgroup.Group{} From 42376f3e91fdb021f8fdbc5e7f849ab8f6493bba Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:53:46 +0200 Subject: [PATCH 5/8] perf(remote): parallelize install validation --- cli/installations.go | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/cli/installations.go b/cli/installations.go index b70792d..d491c86 100644 --- a/cli/installations.go +++ b/cli/installations.go @@ -183,6 +183,8 @@ func (i *Installations) DeleteInstallation(installPath string) error { return nil } +var rootExecutables = []string{"FactoryGame.exe", "FactoryServer.sh", "FactoryServer.exe"} + func (i *Installation) Validate(ctx *GlobalContext) error { found := false for _, p := range ctx.Profiles.Profiles { @@ -203,31 +205,25 @@ func (i *Installation) Validate(ctx *GlobalContext) error { foundExecutable := false - exists, err := d.Exists(filepath.Join(i.BasePath(), "FactoryGame.exe")) - if !exists { - if err != nil { - return fmt.Errorf("failed reading FactoryGame.exe: %w", err) - } - } else { - foundExecutable = true - } + var checkWait errgroup.Group - exists, err = d.Exists(filepath.Join(i.BasePath(), "FactoryServer.sh")) - if !exists { - if err != nil { - return fmt.Errorf("failed reading FactoryServer.sh: %w", err) - } - } else { - foundExecutable = true + for _, executable := range rootExecutables { + e := executable + checkWait.Go(func() error { + exists, err := d.Exists(filepath.Join(i.BasePath(), e)) + if !exists { + if err != nil { + return fmt.Errorf("failed reading %s: %w", e, err) + } + } else { + foundExecutable = true + } + return nil + }) } - exists, err = d.Exists(filepath.Join(i.BasePath(), "FactoryServer.exe")) - if !exists { - if err != nil { - return fmt.Errorf("failed reading FactoryServer.exe: %w", err) - } - } else { - foundExecutable = true + if err = checkWait.Wait(); err != nil { + return err //nolint:wrapcheck } if !foundExecutable { From 84174a75ebae9ad0e945471a32f26523ec74e20e Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 17:54:09 +0200 Subject: [PATCH 6/8] perf(remote): remove unnecessary validation in GetGameVersion --- cli/installations.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/cli/installations.go b/cli/installations.go index d491c86..c79faa8 100644 --- a/cli/installations.go +++ b/cli/installations.go @@ -674,10 +674,6 @@ type gameVersionFile struct { } func (i *Installation) GetGameVersion(ctx *GlobalContext) (int, error) { - if err := i.Validate(ctx); err != nil { - return 0, fmt.Errorf("failed to validate installation: %w", err) - } - platform, err := i.GetPlatform(ctx) if err != nil { return 0, err @@ -689,14 +685,6 @@ func (i *Installation) GetGameVersion(ctx *GlobalContext) (int, error) { } fullPath := filepath.Join(i.BasePath(), platform.VersionPath) - exists, err := d.Exists(fullPath) - if err != nil { - return 0, err - } - - if !exists { - return 0, errors.New("game version file does not exist") - } file, err := d.Read(fullPath) if err != nil { From 94b736093c35a463573ea338c6d7c6d8ba971f99 Mon Sep 17 00:00:00 2001 From: mircearoata Date: Fri, 26 Apr 2024 20:09:26 +0200 Subject: [PATCH 7/8] pref(remote): reduce amount of Validate and GetPlatform calls --- cli/installations.go | 79 +++++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/cli/installations.go b/cli/installations.go index c79faa8..2c91702 100644 --- a/cli/installations.go +++ b/cli/installations.go @@ -239,26 +239,18 @@ var ( matchAllCap = regexp.MustCompile(`([a-z\d])([A-Z])`) ) -func (i *Installation) LockFilePath(ctx *GlobalContext) (string, error) { - platform, err := i.GetPlatform(ctx) - if err != nil { - return "", err - } - +func (i *Installation) lockFilePath(ctx *GlobalContext, platform *Platform) string { lockFileName := ctx.Profiles.Profiles[i.Profile].Name lockFileName = matchFirstCap.ReplaceAllString(lockFileName, "${1}_${2}") lockFileName = matchAllCap.ReplaceAllString(lockFileName, "${1}_${2}") lockFileName = lockFileCleaner.ReplaceAllLiteralString(lockFileName, "-") lockFileName = strings.ToLower(lockFileName) + "-lock.json" - return filepath.Join(i.BasePath(), platform.LockfilePath, lockFileName), nil + return filepath.Join(i.BasePath(), platform.LockfilePath, lockFileName) } -func (i *Installation) LockFile(ctx *GlobalContext) (*resolver.LockFile, error) { - lockfilePath, err := i.LockFilePath(ctx) - if err != nil { - return nil, err - } +func (i *Installation) lockfile(ctx *GlobalContext, platform *Platform) (*resolver.LockFile, error) { + lockfilePath := i.lockFilePath(ctx, platform) d, err := i.GetDisk() if err != nil { @@ -287,11 +279,8 @@ func (i *Installation) LockFile(ctx *GlobalContext) (*resolver.LockFile, error) return lockFile, nil } -func (i *Installation) WriteLockFile(ctx *GlobalContext, lockfile *resolver.LockFile) error { - lockfilePath, err := i.LockFilePath(ctx) - if err != nil { - return err - } +func (i *Installation) writeLockFile(ctx *GlobalContext, platform *Platform, lockfile *resolver.LockFile) error { + lockfilePath := i.lockFilePath(ctx, platform) d, err := i.GetDisk() if err != nil { @@ -337,15 +326,15 @@ func (i *Installation) Wipe() error { return nil } -func (i *Installation) ResolveProfile(ctx *GlobalContext) (*resolver.LockFile, error) { - lockFile, err := i.LockFile(ctx) +func (i *Installation) resolveProfile(ctx *GlobalContext, platform *Platform) (*resolver.LockFile, error) { + lockFile, err := i.lockfile(ctx, platform) if err != nil { return nil, err } depResolver := resolver.NewDependencyResolver(ctx.Provider, viper.GetString("api-base")) - gameVersion, err := i.GetGameVersion(ctx) + gameVersion, err := i.getGameVersion(platform) if err != nil { return nil, fmt.Errorf("failed to detect game version: %w", err) } @@ -355,13 +344,37 @@ func (i *Installation) ResolveProfile(ctx *GlobalContext) (*resolver.LockFile, e return nil, fmt.Errorf("could not resolve mods: %w", err) } - if err := i.WriteLockFile(ctx, lockfile); err != nil { + if err := i.writeLockFile(ctx, platform, lockfile); err != nil { return nil, fmt.Errorf("failed to write lockfile: %w", err) } return lockfile, nil } +func (i *Installation) GetGameVersion(ctx *GlobalContext) (int, error) { + platform, err := i.GetPlatform(ctx) + if err != nil { + return 0, err + } + return i.getGameVersion(platform) +} + +func (i *Installation) LockFile(ctx *GlobalContext) (*resolver.LockFile, error) { + platform, err := i.GetPlatform(ctx) + if err != nil { + return nil, err + } + return i.lockfile(ctx, platform) +} + +func (i *Installation) WriteLockFile(ctx *GlobalContext, lockfile *resolver.LockFile) error { + platform, err := i.GetPlatform(ctx) + if err != nil { + return err + } + return i.writeLockFile(ctx, platform, lockfile) +} + type InstallUpdateType string var ( @@ -383,10 +396,6 @@ type InstallUpdateItem struct { } func (i *Installation) Install(ctx *GlobalContext, updates chan<- InstallUpdate) error { - if err := i.Validate(ctx); err != nil { - return fmt.Errorf("failed to validate installation: %w", err) - } - platform, err := i.GetPlatform(ctx) if err != nil { return fmt.Errorf("failed to detect platform: %w", err) @@ -396,7 +405,7 @@ func (i *Installation) Install(ctx *GlobalContext, updates chan<- InstallUpdate) if !i.Vanilla { var err error - lockfile, err = i.ResolveProfile(ctx) + lockfile, err = i.resolveProfile(ctx, platform) if err != nil { return fmt.Errorf("failed to resolve lockfile: %w", err) } @@ -529,18 +538,19 @@ func (i *Installation) Install(ctx *GlobalContext, updates chan<- InstallUpdate) } func (i *Installation) UpdateMods(ctx *GlobalContext, mods []string) error { - if err := i.Validate(ctx); err != nil { - return fmt.Errorf("failed to validate installation: %w", err) + platform, err := i.GetPlatform(ctx) + if err != nil { + return err } - lockFile, err := i.LockFile(ctx) + lockFile, err := i.lockfile(ctx, platform) if err != nil { return fmt.Errorf("failed to read lock file: %w", err) } resolver := resolver.NewDependencyResolver(ctx.Provider, viper.GetString("api-base")) - gameVersion, err := i.GetGameVersion(ctx) + gameVersion, err := i.getGameVersion(platform) if err != nil { return fmt.Errorf("failed to detect game version: %w", err) } @@ -559,7 +569,7 @@ func (i *Installation) UpdateMods(ctx *GlobalContext, mods []string) error { return fmt.Errorf("failed to resolve dependencies: %w", err) } - if err := i.WriteLockFile(ctx, newLockFile); err != nil { + if err := i.writeLockFile(ctx, platform, newLockFile); err != nil { return fmt.Errorf("failed to write lock file: %w", err) } @@ -673,12 +683,7 @@ type gameVersionFile struct { IsPromotedBuild int `json:"IsPromotedBuild"` } -func (i *Installation) GetGameVersion(ctx *GlobalContext) (int, error) { - platform, err := i.GetPlatform(ctx) - if err != nil { - return 0, err - } - +func (i *Installation) getGameVersion(platform *Platform) (int, error) { d, err := i.GetDisk() if err != nil { return 0, err From dc837046bf7c388d6f7004ea8180d2348327b4a5 Mon Sep 17 00:00:00 2001 From: mircearoata Date: Sun, 28 Apr 2024 15:34:35 +0200 Subject: [PATCH 8/8] chore: remove unnecessary error handling --- cli/disk/ftp.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/cli/disk/ftp.go b/cli/disk/ftp.go index e8884c2..c66119c 100644 --- a/cli/disk/ftp.go +++ b/cli/disk/ftp.go @@ -138,15 +138,11 @@ func (l *ftpDisk) existsWithLock(res *puddle.Resource[*ftp.ServerConn], p string } } else { if errors.As(err, &protocolError) { - switch protocolError.Code { - case ftp.StatusFileUnavailable: + if protocolError.Code == ftp.StatusFileUnavailable { return false, nil - default: - // We won't handle any other kind of error, see above. - return false, fmt.Errorf("failed to list path: %w", err) } } - // This is a non-protocol error, see above. + // We won't handle any other kind of error, see above. return false, fmt.Errorf("failed to list path: %w", err) } @@ -168,17 +164,13 @@ func (l *ftpDisk) existsWithLock(res *puddle.Resource[*ftp.ServerConn], p string } if errors.As(err, &protocolError) { - switch protocolError.Code { - case ftp.StatusFileUnavailable: + if protocolError.Code == ftp.StatusFileUnavailable { return false, nil - default: - // We won't handle any other kind of error, see above. - return false, fmt.Errorf("failed to list path: %w", err) } } - // This is a non-protocol error, see above. - return false, fmt.Errorf("failed to list path: %w", err) + // We won't handle any other kind of error, see above. + return false, fmt.Errorf("failed to list parent path: %w", err) } func (l *ftpDisk) Exists(p string) (bool, error) {