diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index c101abde5e..764b36de2b 100644 --- a/internal/smartlink/smartlink.go +++ b/internal/smartlink/smartlink.go @@ -28,8 +28,12 @@ func LinkContents(src, dest string) error { if err != nil { return errs.Wrap(err, "Reading dir %s failed", src) } + // src and dest are already resolved above, so recurse via link() which does not + // re-resolve each entry. On Windows resolving a path is a syscall (GetLongPathName), + // so resolving once per tree instead of once per file is a large speed-up when + // installing runtimes that contain many files. for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { + if err := link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { return errs.Wrap(err, "Link failed") } } @@ -40,23 +44,37 @@ func LinkContents(src, dest string) error { // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure), // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks. func Link(src, dest string) error { - originalSrc := src + resolvedDest, err := fileutils.ResolveUniquePath(dest) + if err != nil { + return errs.Wrap(err, "Could not resolve dest path") + } - var err error - src, dest, err = resolvePaths(src, dest) + // Resolve the parent of src rather than src itself and rejoin the base name. ResolveUniquePath + // dereferences symlinks, but link() needs to see whether the leaf is itself a symlink, so we must + // not dereference it here. For a non-symlink leaf this is equivalent to resolving src directly. + srcParent, err := fileutils.ResolveUniquePath(filepath.Dir(src)) if err != nil { - return errs.Wrap(err, "Could not resolve src and dest paths") + return errs.Wrap(err, "Could not resolve src path") } + return link(filepath.Join(srcParent, filepath.Base(src)), resolvedDest) +} + +// link recursively links src into dest. Unlike Link, it assumes src and dest are already resolved to +// unique paths and does NOT re-resolve them for every entry it visits. Path resolution is a syscall +// per path on Windows (GetLongPathName), so resolving once per tree rather than once per file makes +// installing runtimes with many files dramatically faster. Descendant paths are constructed by joining +// already-resolved parents with real (long) entry names, so they need no further resolution. +func link(src, dest string) error { if fileutils.IsDir(src) { - if fileutils.IsSymlink(originalSrc) { - // If the original src is a symlink, the resolved src is no longer a symlink and could point + if fileutils.IsSymlink(src) { + // If src is a symlink, the resolved src is no longer a symlink and could point // to a parent directory, resulting in a recursive directory structure. - // Avoid any potential problems by simply linking the original link to the target. + // Avoid any potential problems by simply linking the symlink to the target. // Links to directories are okay on Linux and macOS, but will fail on Windows. // If we ever get here on Windows, the artifact being deployed is bad and there's nothing we // can do about it except receive the report from Rollbar and report it internally. - return linkFile(originalSrc, dest) + return linkFile(src, dest) } if err := fileutils.Mkdir(dest); err != nil { @@ -67,13 +85,22 @@ func Link(src, dest string) error { return errs.Wrap(err, "could not read directory %s", src) } for _, entry := range entries { - if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { + if err := link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil { return errs.Wrap(err, "sub link failed") } } return nil } + // A symlink whose target is a file: link to the resolved target to preserve pre-existing behavior. + if fileutils.IsSymlink(src) { + resolvedSrc, err := fileutils.ResolveUniquePath(src) + if err != nil { + return errs.Wrap(err, "could not resolve src path %s", src) + } + src = resolvedSrc + } + destDir := filepath.Dir(dest) if err := fileutils.MkdirUnlessExists(destDir); err != nil { return errs.Wrap(err, "could not create directory %s", destDir) @@ -87,6 +114,12 @@ func Link(src, dest string) error { } if err := linkFile(src, dest); err != nil { + // Another artifact may have created the same file concurrently between the check above and now. + // Treat that the same as the "already exists" case rather than failing the whole install. + if os.IsExist(err) { + logging.Warning("Skipping linking '%s' to '%s' as it already exists", src, dest) + return nil + } return errs.Wrap(err, "could not link %s to %s", src, dest) } return nil diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index a558400442..ddfd477a13 100644 --- a/pkg/runtime/depot.go +++ b/pkg/runtime/depot.go @@ -214,10 +214,14 @@ func (d *depot) MarkPrivate(id strfmt.UUID) error { // DeployViaLink will take an artifact from the depot and link it to the target path. // It should return deployment info to be used for tracking the artifact. +// +// Unlike DeployViaCopy this does NOT hold d.fsMutex for the duration of the operation, so multiple +// artifacts can be linked into the runtime concurrently (the install worker pool runs several at once). +// This is safe because linking touches no shared in-memory depot state (d.Exists guards its own access +// via mapMutex, and d.depotPath is immutable after construction), directory creation is idempotent, and +// smartlink treats an already-existing target as a skip. This is a major win on Windows where each file +// link is an individual, relatively expensive syscall. func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) (*deployment, error) { - d.fsMutex.Lock() - defer d.fsMutex.Unlock() - if exists, _ := d.Exists(id); !exists { return nil, errs.New("artifact not found in depot") } @@ -262,6 +266,11 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) // DeployViaCopy will take an artifact from the depot and copy it to the target path. // It should return deployment info to be used for tracking the artifact. +// +// This intentionally holds d.fsMutex for the duration of the copy. Unlike linking, copying a file that +// two artifacts both provide would race on os.Create and could corrupt the destination, so copies are +// serialized. The copy path is only used when an artifact needs file transforms, hard links are +// unsupported, or portable mode is requested, so it is the less common case. func (d *depot) DeployViaCopy(id strfmt.UUID, relativeSrc, absoluteDest string) (*deployment, error) { d.fsMutex.Lock() defer d.fsMutex.Unlock()