Allow files.rm_recursive to yield whenever possible.

A while back, for performance reasons, we disabled yielding for all
files.* operations unless METEOR_DISABLE_FS_FIBERS was set to false.

This was safe for almost all files.* operations, because most of them have
a synchronous fs.*Sync version available.

For a more complicated operation like files.rm_recursive, however, there
is no synchronous or asynchronous counterpart in the fs.* namespace, so
the safety of disabling fibers is not guaranteed.

Lately, files.rm_recursive has become a major source of uncaught ENOTEMPTY
errors on Windows, because rimraf.sync fails with that error, and we don't
give files.rm_recursive_async a chance to delete the directory in a more
persistent, forgiving manner.

The only reason we haven't been falling back to files.rm_recursive_async
is that YIELD_ALLOWED is false by default, so canYield() returns false.

This commit distinguishes between canYield() and mayYield(), and uses
canYield() in files.rm_recursive to determine whether it is technically
safe to yield, regardless of YIELD_ALLOWED.

Anyone who ever asked "Can I go to the bathroom?" in elementary school,
only to be mercilessly rebuked with "I don't know, CAN YOU?" should
understand the difference between these two functions.
This commit is contained in:
Ben Newman
2017-10-16 13:19:02 -04:00
parent af016aa306
commit e6e5d427b4

View File

@@ -55,12 +55,15 @@ const YIELD_ALLOWED = !! (
! JSON.parse(process.env.METEOR_DISABLE_FS_FIBERS));
function canYield() {
return YIELD_ALLOWED &&
Fiber.current &&
return Fiber.current &&
Fiber.yield &&
! Fiber.yield.disallowed;
}
function mayYield() {
return YIELD_ALLOWED && canYield();
}
// given a predicate function and a starting path, traverse upwards
// from the path until we find a path that satisfies the predicate.
//
@@ -303,8 +306,7 @@ files.rm_recursive = Profile("files.rm_recursive", (path) => {
try {
rimraf.sync(files.convertToOSPath(path));
} catch (e) {
if (e.code === "ENOTEMPTY" &&
canYield()) {
if (e.code === "ENOTEMPTY" && canYield()) {
files.rm_recursive_async(path).await();
return;
}
@@ -1533,7 +1535,7 @@ function wrapFsFunc(fsFuncName, pathArgIndices, options) {
const dirty = options && options.dirty;
const dirtyFn = typeof dirty === "function" ? dirty : null;
if (canYield() &&
if (mayYield() &&
shouldBeSync &&
! isQuickie) {
const promise = new Promise((resolve, reject) => {