Bug 961317 - Clean up OS.File shutdown race condition and rework OS.File reset/shutdown. r=froydnj, a=lsblakk
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Wed, 02 Apr 2014 11:42:51 -0400
changeset 192696 8e92bb5cb712986be5e0cda4a653fea6fa3c3a81
parent 192695 1e80ec83f5b320dbdcf5a9400d508effd07281f7
child 192697 59fb1b1b8d57a35fdf98dee96c3ae4801529325f
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, lsblakk
bugs961317
milestone30.0a2
Bug 961317 - Clean up OS.File shutdown race condition and rework OS.File reset/shutdown. r=froydnj, a=lsblakk
toolkit/components/osfile/modules/osfile_async_front.jsm
toolkit/components/osfile/modules/osfile_async_worker.js
toolkit/components/osfile/tests/xpcshell/test_reset.js
toolkit/components/osfile/tests/xpcshell/test_shutdown.js
--- a/toolkit/components/osfile/modules/osfile_async_front.jsm
+++ b/toolkit/components/osfile/modules/osfile_async_front.jsm
@@ -171,29 +171,123 @@ let Scheduler = {
    */
   latestReceived: undefined,
 
   /**
    * A timer used to automatically shut down the worker after some time.
    */
   resetTimer: null,
 
+  /**
+   * Prepare to kill the OS.File worker after a few seconds.
+   */
   restartTimer: function(arg) {
     let delay;
     try {
       delay = Services.prefs.getIntPref("osfile.reset_worker_delay");
     } catch(e) {
       // Don't auto-shutdown if we don't have a delay preference set.
       return;
     }
 
     if (this.resetTimer) {
       clearTimeout(this.resetTimer);
     }
-    this.resetTimer = setTimeout(File.resetWorker, delay);
+    this.resetTimer = setTimeout(
+      () => Scheduler.kill({reset: true, shutdown: false}),
+      delay
+    );
+  },
+
+  /**
+   * Shutdown OS.File.
+   *
+   * @param {*} options
+   * - {boolean} shutdown If |true|, reject any further request. Otherwise,
+   *   further requests will resurrect the worker.
+   * - {boolean} reset If |true|, instruct the worker to shutdown if this
+   *   would not cause leaks. Otherwise, assume that the worker will be shutdown
+   *   through some other mean.
+   */
+  kill: function({shutdown, reset}) {
+    return Task.spawn(function*() {
+
+      yield this.queue;
+
+      if (!this.launched || this.shutdown || !worker) {
+        // Nothing to kill
+        this.shutdown = this.shutdown || shutdown;
+        worker = null;
+        return null;
+      }
+
+      // Deactivate the queue, to ensure that no message is sent
+      // to an obsolete worker (we reactivate it in the |finally|).
+      let deferred = Promise.defer();
+      this.queue = deferred.promise;
+
+      let message = ["Meta_shutdown", [reset]];
+
+      try {
+        Scheduler.latestReceived = [];
+        Scheduler.latestSent = [Date.now(), ...message];
+        let promise = worker.post(...message);
+
+        // Wait for result
+        let resources;
+        try {
+          resources = (yield promise).ok;
+
+          Scheduler.latestReceived = [Date.now(), message];
+        } catch (ex) {
+          LOG("Could not dispatch Meta_reset", ex);
+          // It's most likely a programmer error, but we'll assume that
+          // the worker has been shutdown, as it's less risky than the
+          // opposite stance.
+          resources = {openedFiles: [], openedDirectoryIterators: [], killed: true};
+
+          Scheduler.latestReceived = [Date.now(), message, ex];
+        }
+
+        let {openedFiles, openedDirectoryIterators, killed} = resources;
+        if (!reset
+          && (openedFiles && openedFiles.length
+            || ( openedDirectoryIterators && openedDirectoryIterators.length))) {
+          // The worker still holds resources. Report them.
+
+          let msg = "";
+          if (openedFiles.length > 0) {
+            msg += "The following files are still open:\n" +
+              openedFiles.join("\n");
+          }
+          if (openedDirectoryIterators.length > 0) {
+            msg += "The following directory iterators are still open:\n" +
+              openedDirectoryIterators.join("\n");
+          }
+
+          LOG("WARNING: File descriptors leaks detected.\n" + msg);
+        }
+
+        // Make sure that we do not leave an invalid |worker| around.
+        if (killed || shutdown) {
+          worker = null;
+        }
+
+        this.shutdown = shutdown;
+
+        return resources;
+
+      } finally {
+        // Resume accepting messages. If we have set |shutdown| to |true|,
+        // any pending/future request will be rejected. Otherwise, any
+        // pending/future request will spawn a new worker if necessary.
+        deferred.resolve();
+      }
+
+    }.bind(this));
   },
 
   /**
    * Push a task at the end of the queue.
    *
    * @param {function} code A function returning a Promise.
    * This function will be executed once all the previously
    * pushed tasks have completed.
@@ -245,16 +339,20 @@ let Scheduler = {
     return this.push(() => Task.spawn(function*() {
       Scheduler.latestReceived = null;
       if (OS.Constants.Sys.DEBUG) {
         // Update possibly memory-expensive debugging information
         Scheduler.latestSent = [Date.now(), method, ...args];
       } else {
         Scheduler.latestSent = [Date.now(), method];
       }
+
+      // Don't kill the worker just yet
+      Scheduler.restartTimer();
+
       let data;
       let reply;
       let isError = false;
       try {
         data = yield worker.post(method, ...args);
         reply = data;
       } catch (error if error instanceof PromiseWorker.WorkerError) {
         reply = error;
@@ -277,21 +375,17 @@ let Scheduler = {
           Scheduler.latestReceived = [Date.now(), reply.message, reply.fileName, reply.lineNumber];
         } else {
           Scheduler.latestReceived = [Date.now()];
         }
         if (firstLaunch) {
           Scheduler._updateTelemetry();
         }
 
-        // Don't restart the timer when reseting the worker, since that will
-        // lead to an endless "resetWorker()" loop.
-        if (method != "Meta_reset") {
-          Scheduler.restartTimer();
-        }
+        Scheduler.restartTimer();
       }
 
       // Check for duration and return result.
       if (!options) {
         return data.ok;
       }
       // Check for options.outExecutionDuration.
       if (typeof options !== "object" ||
@@ -401,64 +495,19 @@ if (SharedAll.Config.DEBUG && Scheduler.
 
 // Observer topics used for monitoring shutdown
 const WEB_WORKERS_SHUTDOWN_TOPIC = "web-workers-shutdown";
 
 // Preference used to configure test shutdown observer.
 const PREF_OSFILE_TEST_SHUTDOWN_OBSERVER =
   "toolkit.osfile.test.shutdown.observer";
 
-/**
- * A condition function meant to be used during phase
- * webWorkersShutdown, to warn about unclosed files and directories
- * and reconfigure the Scheduler to reject further requests.
- *
- * @param {bool=} shutdown If true or unspecified, reconfigure
- * the scheduler to reject further requests. Can be set to |false|
- * for testing purposes.
- * @return {promise} A promise satisfied once all pending messages
- * (including the shutdown warning message) have been answered.
- */
-function warnAboutUnclosedFiles(shutdown = true) {
-  if (!Scheduler.launched || !worker) {
-    // Don't launch the scheduler on our behalf. If no message has been
-    // sent to the worker, we can't have any leaking file/directory
-    // descriptor.
-    return null;
-  }
-  let promise = Scheduler.post("Meta_getUnclosedResources");
-
-  // Configure the worker to reject any further message.
-  if (shutdown) {
-    Scheduler.shutdown = true;
-  }
-
-  return promise.then(function onSuccess(opened) {
-    let msg = "";
-    if (opened.openedFiles.length > 0) {
-      msg += "The following files are still open:\n" +
-        opened.openedFiles.join("\n");
-    }
-    if (msg) {
-      msg += "\n";
-    }
-    if (opened.openedDirectoryIterators.length > 0) {
-      msg += "The following directory iterators are still open:\n" +
-        opened.openedDirectoryIterators.join("\n");
-    }
-    // Only log if file descriptors leaks detected.
-    if (msg) {
-      LOG("WARNING: File descriptors leaks detected.\n" + msg);
-    }
-  });
-};
-
 AsyncShutdown.webWorkersShutdown.addBlocker(
   "OS.File: flush pending requests, warn about unclosed files, shut down service.",
-  () => warnAboutUnclosedFiles(true)
+  () => Scheduler.kill({reset: false, shutdown: true})
 );
 
 
 // Attaching an observer for PREF_OSFILE_TEST_SHUTDOWN_OBSERVER to enable or
 // disable the test shutdown event observer.
 // Note: By default the PREF_OSFILE_TEST_SHUTDOWN_OBSERVER is unset.
 // Note: This is meant to be used for testing purposes only.
 Services.prefs.addObserver(PREF_OSFILE_TEST_SHUTDOWN_OBSERVER,
@@ -473,17 +522,17 @@ Services.prefs.addObserver(PREF_OSFILE_T
     }
     if (TOPIC) {
       // Generate a phase, add a blocker.
       // Note that this can work only if AsyncShutdown itself has been
       // configured for testing by the testsuite.
       let phase = AsyncShutdown._getPhase(TOPIC);
       phase.addBlocker(
         "(for testing purposes) OS.File: warn about unclosed files",
-        () => warnAboutUnclosedFiles(false)
+        () => Scheduler.kill({shutdown: false, reset: false})
       );
     }
   }, false);
 
 /**
  * Representation of a file, with asynchronous methods.
  *
  * @param {*} fdmsg The _message_ representing the platform-specific file
@@ -1267,54 +1316,25 @@ DirectoryIterator.Entry = function Entry
   return value;
 };
 DirectoryIterator.Entry.prototype = Object.create(SysAll.AbstractEntry.prototype);
 
 DirectoryIterator.Entry.fromMsg = function fromMsg(value) {
   return new DirectoryIterator.Entry(value);
 };
 
-/**
- * Flush all operations currently queued, then kill the underlying
- * worker to save memory.
- *
- * @return {Promise}
- * @reject {Error} If at least one file or directory iterator instance
- * is still open and the worker cannot be killed safely.
- */
 File.resetWorker = function() {
-  if (!Scheduler.launched || Scheduler.shutdown) {
-    // No need to reset
-    return Promise.resolve();
-  }
-  return Scheduler.post("Meta_reset").then(
-    function(wouldLeak) {
-      if (!wouldLeak) {
-        // No resource would leak, the worker was stopped.
-        worker = null;
-        return;
-      }
-      // Otherwise, resetting would be unsafe and has been canceled.
-      // Turn this into an error
-      let msg = "Cannot reset worker: ";
-      let {openedFiles, openedDirectoryIterators} = wouldLeak;
-      if (openedFiles.length > 0) {
-        msg += "The following files are still open:\n" +
-          openedFiles.join("\n");
-      }
-      if (openedDirectoryIterators.length > 0) {
-        msg += "The following directory iterators are still open:\n" +
-          openedDirectoryIterators.join("\n");
-      }
-      throw new Error(msg);
+  return Task.spawn(function*() {
+    let resources = yield Scheduler.kill({shutdown: false, reset: true});
+    if (resources && !resources.killed) {
+        throw new Error("Could not reset worker, this would leak file descriptors: " + JSON.stringify(resources));
     }
-  );
+  });
 };
 
-
 // Constants
 File.POS_START = SysAll.POS_START;
 File.POS_CURRENT = SysAll.POS_CURRENT;
 File.POS_END = SysAll.POS_END;
 
 // Exports
 File.Error = OSError;
 File.DirectoryIterator = DirectoryIterator;
--- a/toolkit/components/osfile/modules/osfile_async_worker.js
+++ b/toolkit/components/osfile/modules/osfile_async_worker.js
@@ -267,44 +267,35 @@ const EXCEPTION_NAMES = {
    SET_DEBUG: function(aDEBUG) {
      SharedAll.Config.DEBUG = aDEBUG;
    },
    // Return worker's current OS.Shared.DEBUG value to controller.
    // Note: This is used for testing purposes.
    GET_DEBUG: function() {
      return SharedAll.Config.DEBUG;
    },
-   Meta_getUnclosedResources: function() {
-     // Return information about both opened files and opened
-     // directory iterators.
-     return {
+   /**
+    * Execute shutdown sequence, returning data on leaked file descriptors.
+    *
+    * @param {bool} If |true|, kill the worker if this would not cause
+    * leaks.
+    */
+   Meta_shutdown: function(kill) {
+     let result = {
        openedFiles: OpenedFiles.listOpenedResources(),
-       openedDirectoryIterators: OpenedDirectoryIterators.listOpenedResources()
+       openedDirectoryIterators: OpenedDirectoryIterators.listOpenedResources(),
+       killed: false // Placeholder
      };
-   },
-   Meta_reset: function() {
-     // Attempt to stop the worker. This fails if at least one
-     // resource is still open. Returns the list of files and
-     // directory iterators that cannot be closed safely (or undefined
-     // if there are no such files/directory iterators).
-     let openedFiles = OpenedFiles.listOpenedResources();
-     let openedDirectoryIterators =
-       OpenedDirectoryIterators.listOpenedResources();
-     let canShutdown = openedFiles.length == 0
-                         && openedDirectoryIterators.length == 0;
-     if (canShutdown) {
-       // Succeed. Shutdown the thread, nothing to return
-       return new Meta(null, {shutdown: true});
-     } else {
-       // Fail. Don't shutdown the thread, return info on resources
-       return {
-         openedFiles: openedFiles,
-         openedDirectoryIterators: openedDirectoryIterators
-       };
-     }
+
+     // Is it safe to kill the worker?
+     let safe = result.openedFiles.length == 0
+           && result.openedDirectoryIterators.length == 0;
+     result.killed = safe && kill;
+
+     return new Meta(result, {shutdown: result.killed});
    },
    // Functions of OS.File
    stat: function stat(path) {
      return exports.OS.File.Info.toMsg(
        exports.OS.File.stat(Type.path.fromMsg(path)));
    },
    setDates: function setDates(path, accessDate, modificationDate) {
      return exports.OS.File.setDates(Type.path.fromMsg(path), accessDate,
--- a/toolkit/components/osfile/tests/xpcshell/test_reset.js
+++ b/toolkit/components/osfile/tests/xpcshell/test_reset.js
@@ -32,34 +32,34 @@ add_task(function transparent_reset() {
 
 add_task(function file_open_cannot_reset() {
   let TEST_FILE = OS.Path.join(Path.profileDir, "tmp-" + Math.random());
   do_print("Leaking file descriptor " + TEST_FILE + ", we shouldn't be able to reset");
   let openedFile = yield OS.File.open(TEST_FILE, { create: true} );
   let thrown = false;
   try {
     yield OS.File.resetWorker();
-  } catch (ex if ex.message.indexOf(TEST_FILE) != -1 ) {
+  } catch (ex if ex.message.indexOf(OS.Path.basename(TEST_FILE)) != -1 ) {
     thrown = true;
   }
   do_check_true(thrown);
 
   do_print("Closing the file, we should now be able to reset");
   yield openedFile.close();
   yield OS.File.resetWorker();
 });
 
-add_task(function file_open_cannot_reset() {
+add_task(function dir_open_cannot_reset() {
   let TEST_DIR = yield OS.File.getCurrentDirectory();
   do_print("Leaking directory " + TEST_DIR + ", we shouldn't be able to reset");
   let iterator = new OS.File.DirectoryIterator(TEST_DIR);
   let thrown = false;
   try {
     yield OS.File.resetWorker();
-  } catch (ex if ex.message.indexOf(TEST_DIR) != -1 ) {
+  } catch (ex if ex.message.indexOf(OS.Path.basename(TEST_DIR)) != -1 ) {
     thrown = true;
   }
   do_check_true(thrown);
 
   do_print("Closing the directory, we should now be able to reset");
   yield iterator.close();
   yield OS.File.resetWorker();
 });
--- a/toolkit/components/osfile/tests/xpcshell/test_shutdown.js
+++ b/toolkit/components/osfile/tests/xpcshell/test_shutdown.js
@@ -84,15 +84,15 @@ add_task(function system_shutdown() {
 
   let TEST_FILE = OS.Path.join(OS.Constants.Path.profileDir, "test");
   do_print("Testing for leaks of file descriptor: " + TEST_FILE);
   let openedFile = yield OS.File.open(TEST_FILE, { create: true} );
   do_print("At this stage, we leak the file");
   do_check_true((yield testLeaksOf(TEST_FILE, "test.shutdown.file.leak")));
   yield openedFile.close();
   do_print("At this stage, we don't leak the file anymore");
-  do_check_false((yield testLeaksOf(TEST_FILE, "test.shutdown.file.leak")));
+  do_check_false((yield testLeaksOf(TEST_FILE, "test.shutdown.file.leak.2")));
 });
 
 
 function run_test() {
   run_next_test();
 }