From e6e5d427b48536d3924facd7e89bca896bb155c8 Mon Sep 17 00:00:00 2001
From: Ben Newman <ben@meteor.com>
Date: Mon, 16 Oct 2017 13:19:02 -0400
Subject: [PATCH] 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.
---
 tools/fs/files.js | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/fs/files.js b/tools/fs/files.js
index 18c97e41a3..86bb4932db 100644
--- a/tools/fs/files.js
+++ b/tools/fs/files.js
@@ -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) => {