From fcaf11a29c421b0e76c9f24f5b155a53eff14e7f Mon Sep 17 00:00:00 2001
From: Shelley Vohr <codebytere@github.com>
Date: Fri, 28 Sep 2018 10:39:20 -0700
Subject: [PATCH] fix: do not crash when using a closed fs event watcher (#70)

---
 doc/api/fs.md                                 |  6 ++-
 lib/fs.js                                     | 17 +++++++--
 .../test-fs-watch-close-when-destroyed.js     | 38 +++++++++++++++++++
 test/parallel/test-fs-watch.js                | 19 ++++++++--
 4 files changed, 70 insertions(+), 10 deletions(-)
 create mode 100644 test/parallel/test-fs-watch-close-when-destroyed.js

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 26eda38f739892..30ee959928490f 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -325,7 +325,8 @@ fs.watch('./tmp', { encoding: 'buffer' }, (eventType, filename) => {
 added: v10.0.0
 -->
 
-Emitted when the watcher stops watching for changes.
+Emitted when the watcher stops watching for changes. The closed
+`fs.FSWatcher` object is no longer usable in the event handler.
 
 ### Event: 'error'
 <!-- YAML
@@ -334,7 +335,8 @@ added: v0.5.8
 
 * `error` {Error}
 
-Emitted when an error occurs while watching the file.
+Emitted when the watcher stops watching for changes. The errored
+`fs.FSWatcher` object is no longer usable in the event handler.
 
 ### watcher.close()
 <!-- YAML
diff --git a/lib/fs.js b/lib/fs.js
index f8ab1bbd5812b7..ea7d34295bfd9f 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1380,7 +1380,11 @@ function FSWatcher() {
     // after the handle is closed, and to fire both UV_RENAME and UV_CHANGE
     // if they are set by libuv at the same time.
     if (status < 0) {
-      self._handle.close();
+      if (this._handle !== null) {
+        // We don't use this.close() here to avoid firing the close event.
+        this._handle.close();
+        this._handle = null;  // make the handle garbage collectable
+      }
       const error = errors.uvException({
         errno: status,
         syscall: 'watch',
@@ -1400,13 +1404,14 @@ util.inherits(FSWatcher, EventEmitter);
 // 1. Throw an Error if it's the first time .start() is called
 // 2. Return silently if .start() has already been called
 //    on a valid filename and the wrap has been initialized
+// 3. Return silently if the watcher has already been closed
 // This method is a noop if the watcher has already been started.
 FSWatcher.prototype.start = function(filename,
                                      persistent,
                                      recursive,
                                      encoding) {
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
-  if (this._handle.initialized) {
+  if (this._handle.initialized) {  // already started
     return;
   }
 
@@ -1428,10 +1433,14 @@ FSWatcher.prototype.start = function(filename,
   }
 };
 
-// This method is a noop if the watcher has not been started.
+// This method is a noop if the watcher has not been started or
+// has already been closed.
 FSWatcher.prototype.close = function() {
+  if (this._handle === null) {  // closed
+    return;
+  }
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
-  if (!this._handle.initialized) {
+  if (!this._handle.initialized) {  // not started
     return;
   }
   this._handle.close();
diff --git a/test/parallel/test-fs-watch-close-when-destroyed.js b/test/parallel/test-fs-watch-close-when-destroyed.js
new file mode 100644
index 00000000000000..e2d5c12123b81c
--- /dev/null
+++ b/test/parallel/test-fs-watch-close-when-destroyed.js
@@ -0,0 +1,38 @@
+'use strict';
+
+// This tests that closing a watcher when the underlying handle is
+// already destroyed will result in a noop instead of a crash.
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const fs = require('fs');
+const path = require('path');
+
+tmpdir.refresh();
+const root = path.join(tmpdir.path, 'watched-directory');
+fs.mkdirSync(root);
+
+const watcher = fs.watch(root, { persistent: false, recursive: false });
+
+// The following listeners may or may not be invoked.
+
+watcher.addListener('error', () => {
+  setTimeout(
+    () => { watcher.close(); },  // Should not crash if it's invoked
+    common.platformTimeout(10)
+  );
+});
+
+watcher.addListener('change', () => {
+  setTimeout(
+    () => { watcher.close(); },
+    common.platformTimeout(10)
+  );
+});
+
+fs.rmdirSync(root);
+// Wait for the listener to hit
+setTimeout(
+  common.mustCall(() => {}),
+  common.platformTimeout(100)
+);
\ No newline at end of file
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
index ffa82e52c73b72..c7171055260a55 100644
--- a/test/parallel/test-fs-watch.js
+++ b/test/parallel/test-fs-watch.js
@@ -46,7 +46,8 @@ for (const testCase of cases) {
   fs.writeFileSync(testCase.filePath, content1);
 
   let interval;
-  const watcher = fs.watch(testCase[testCase.field]);
+  const pathToWatch = testCase[testCase.field];
+  const watcher = fs.watch(pathToWatch);
   watcher.on('error', (err) => {
     if (interval) {
       clearInterval(interval);
@@ -54,7 +55,11 @@ for (const testCase of cases) {
     }
     assert.fail(err);
   });
-  watcher.on('close', common.mustCall());
+  watcher.on('close', common.mustCall(() => {
+    watcher.close(); // Closing a closed watcher should be a noop
+    // Starting a closed watcher should be a noop
+    watcher.start();
+  }));
   watcher.on('change', common.mustCall(function(eventType, argFilename) {
     if (interval) {
       clearInterval(interval);
@@ -66,10 +71,16 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    watcher.start(); // Starting a started watcher should be a noop
-    // End of test case
+    // Starting a started watcher should be a noop
+    watcher.start();
+    watcher.start(pathToWatch);
+    
     watcher.close();
+    
+    // We document that watchers cannot be used anymore when it's closed,
+    // here we turn the methods into noops instead of throwing
     watcher.close(); // Closing a closed watcher should be a noop
+    watcher.start();  // Starting a closed watcher should be a noop
   }));
 
   // Long content so it's actually flushed. toUpperCase so there's real change.