fix handling of concurrent package installs#756
Conversation
aronatkins
left a comment
aronatkins
left a comment
There was a problem hiding this comment.
Thanks so much for improving this cache population logic; it has been a source of fragility.
| tempPath <- tempfile(tmpdir = dirname(cachedPackagePath)) | ||
| on.exit(unlink(tempPath, recursive = TRUE), add = TRUE) | ||
| if (all(dir_copy(packagePath, tempPath))) { | ||
| # check to see if the cached package path exists now; if it does, |
There was a problem hiding this comment.
The dir_copy is a comparatively slow operation and likely to see two processes racing. Unfortunately, because there is time between file.exists and file.rename, our previous code resolved the most common type of races, but did not eliminate them.
Because file.rename is atomic, using it alone is a better guard than our previous file.exists before file.rename.
👍
| } | ||
|
|
||
| # attempt to rename to cache | ||
| if (suppressWarnings(file.rename(packagePath, cachedPackagePath))) { |
There was a problem hiding this comment.
Note: This earlier rename always fails in Connect deployments because the R library and cache are beneath separate filesystems (mounts).
|
|
||
| # back up a pre-existing cached package (restore on failure) | ||
| if (file.exists(cachedPackagePath)) { | ||
| if (!file.rename(cachedPackagePath, backupPackagePath)) { |
There was a problem hiding this comment.
Note: This earlier rename is both good (removing a potentially old cache entry so it can be replaced with just-rebuilt package contents) and bad (many processes may see the need to build this package, separately call moveInstalledPackageToCache and move aside just-created cache entries).
Everything after the cache checks in installPkg leads to these races that we're resolving, because package-building and cache-population are expensive, which makes it more likely that multiple restore operations are doing that work at the same time.
Have you explored writing "burn in" tests of installPkg to see what other problems still linger? This early rename concerns me; maybe overwrite = TRUE is a mistake?
3 participants
Concurrent R processes installing the same package into a shared package cache can collide in
moveInstalledPackageToCache(), where the losing process fails with "failed to restore package ... may be lost from cache".The function already attempted to handle this with a
file.exists()check before renaming into the cache, but a competing process can populate the cache entry between the check and the rename. This PR moves the check to after the rename fails, where there is no window.Also, only roll back a backed-up cache entry when a backup was actually made. The rollback previously ran unconditionally, so a fresh package that failed to insert reported "may be lost from cache" instead of "failed to copy package to cache".
The race window is extremely small, so the tests mock
file.renameto simulate the losing interleaving.