diff --git a/doc/api/errors.md b/doc/api/errors.md index 70ac01de610d6a..7d6e238f93bdaa 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -783,6 +783,18 @@ falsy value. An invalid symlink type was passed to the [`fs.symlink()`][] or [`fs.symlinkSync()`][] methods. + +### ERR_FS_WATCHER_ALREADY_STARTED + +An attempt was made to start a watcher returned by `fs.watch()` that has +already been started. + + +### ERR_FS_WATCHER_NOT_STARTED + +An attempt was made to initiate operations on a watcher returned by +`fs.watch()` that has not yet been started. + ### ERR_HTTP_HEADERS_SENT diff --git a/lib/fs.js b/lib/fs.js index 917c3eb3a9f640..321fbe8d54ef8e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -77,13 +77,19 @@ Object.defineProperty(exports, 'constants', { value: constants }); +let assert_ = null; +function lazyAssert() { + if (assert_ === null) { + assert_ = require('assert'); + } + return assert_; +} + const kMinPoolSpace = 128; const { kMaxLength } = require('buffer'); const isWindows = process.platform === 'win32'; -const errnoException = errors.errnoException; - let truncateWarn = true; function showTruncateDeprecation() { @@ -1312,11 +1318,17 @@ function FSWatcher() { this._handle.owner = this; this._handle.onchange = function(status, eventType, filename) { + // TODO(joyeecheung): we may check self._handle.initialized here + // and return if that is false. This allows us to avoid firing the event + // 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(); - const error = !filename ? - errnoException(status, 'Error watching file for changes:') : - errnoException(status, `Error watching file ${filename} for changes:`); + const error = errors.uvException({ + errno: status, + syscall: 'watch', + path: filename + }); error.filename = filename; self.emit('error', error); } else { @@ -1335,21 +1347,34 @@ FSWatcher.prototype.start = function(filename, persistent, recursive, encoding) { + lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent'); + if (this._handle.initialized) { + throw new errors.Error('ERR_FS_WATCHER_ALREADY_STARTED'); + } + filename = getPathFromURL(filename); - nullCheck(filename, 'filename'); + validatePath(filename, 'filename'); + var err = this._handle.start(pathModule.toNamespacedPath(filename), persistent, recursive, encoding); if (err) { - this._handle.close(); - const error = errnoException(err, `watch ${filename}`); + const error = errors.uvException({ + errno: err, + syscall: 'watch', + path: filename + }); error.filename = filename; throw error; } }; FSWatcher.prototype.close = function() { + lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent'); + if (!this._handle.initialized) { + throw new errors.Error('ERR_FS_WATCHER_NOT_STARTED'); + } this._handle.close(); }; diff --git a/lib/internal/errors.js b/lib/internal/errors.js index d5a5c4fc599619..fd93547d26d37e 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -658,6 +658,10 @@ E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value', Error); E('ERR_FS_INVALID_SYMLINK_TYPE', 'Symlink type must be one of "dir", "file", or "junction". Received "%s"', Error); // Switch to TypeError. The current implementation does not seem right +E('ERR_FS_WATCHER_ALREADY_STARTED', + 'The watcher has already been started', Error); +E('ERR_FS_WATCHER_NOT_STARTED', + 'The watcher has not been started', Error); E('ERR_HTTP2_ALTSVC_INVALID_ORIGIN', 'HTTP/2 ALTSVC frames require a valid origin', TypeError); E('ERR_HTTP2_ALTSVC_LENGTH', diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 85a09060a11edc..b3fa3e8d9a075b 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -31,6 +31,8 @@ namespace node { using v8::Context; +using v8::DontDelete; +using v8::DontEnum; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; using v8::HandleScope; @@ -38,6 +40,9 @@ using v8::Integer; using v8::Local; using v8::MaybeLocal; using v8::Object; +using v8::PropertyAttribute; +using v8::ReadOnly; +using v8::Signature; using v8::String; using v8::Value; @@ -51,7 +56,7 @@ class FSEventWrap: public HandleWrap { static void New(const FunctionCallbackInfo& args); static void Start(const FunctionCallbackInfo& args); static void Close(const FunctionCallbackInfo& args); - + static void GetInitialized(const FunctionCallbackInfo& args); size_t self_size() const override { return sizeof(*this); } private: @@ -80,6 +85,11 @@ FSEventWrap::~FSEventWrap() { CHECK_EQ(initialized_, false); } +void FSEventWrap::GetInitialized(const FunctionCallbackInfo& args) { + FSEventWrap* wrap = Unwrap(args.This()); + CHECK(wrap != nullptr); + args.GetReturnValue().Set(wrap->initialized_); +} void FSEventWrap::Initialize(Local target, Local unused, @@ -95,6 +105,18 @@ void FSEventWrap::Initialize(Local target, env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "close", Close); + Local get_initialized_templ = + FunctionTemplate::New(env->isolate(), + GetInitialized, + env->as_external(), + Signature::New(env->isolate(), t)); + + t->PrototypeTemplate()->SetAccessorProperty( + FIXED_ONE_BYTE_STRING(env->isolate(), "initialized"), + get_initialized_templ, + Local(), + static_cast(ReadOnly | DontDelete | v8::DontEnum)); + target->Set(fsevent_string, t->GetFunction()); } @@ -105,22 +127,19 @@ void FSEventWrap::New(const FunctionCallbackInfo& args) { new FSEventWrap(env, args.This()); } - +// wrap.start(filename, persistent, recursive, encoding) void FSEventWrap::Start(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - FSEventWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - if (wrap->initialized_) - return args.GetReturnValue().Set(0); + FSEventWrap* wrap = Unwrap(args.Holder()); + CHECK_NE(wrap, nullptr); + CHECK(!wrap->initialized_); - static const char kErrMsg[] = "filename must be a string or Buffer"; - if (args.Length() < 1) - return env->ThrowTypeError(kErrMsg); + const int argc = args.Length(); + CHECK_GE(argc, 4); BufferValue path(env->isolate(), args[0]); - if (*path == nullptr) - return env->ThrowTypeError(kErrMsg); + CHECK_NE(*path, nullptr); unsigned int flags = 0; if (args[2]->IsTrue()) @@ -129,19 +148,21 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { wrap->encoding_ = ParseEncoding(env->isolate(), args[3], kDefaultEncoding); int err = uv_fs_event_init(wrap->env()->event_loop(), &wrap->handle_); - if (err == 0) { - wrap->initialized_ = true; + if (err != 0) { + return args.GetReturnValue().Set(err); + } - err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); + err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags); + wrap->initialized_ = true; - if (err == 0) { - // Check for persistent argument - if (!args[1]->IsTrue()) { - uv_unref(reinterpret_cast(&wrap->handle_)); - } - } else { - FSEventWrap::Close(args); - } + if (err != 0) { + FSEventWrap::Close(args); + return args.GetReturnValue().Set(err); + } + + // Check for persistent argument + if (!args[1]->IsTrue()) { + uv_unref(reinterpret_cast(&wrap->handle_)); } args.GetReturnValue().Set(err); @@ -209,13 +230,11 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename, void FSEventWrap::Close(const FunctionCallbackInfo& args) { - FSEventWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + FSEventWrap* wrap = Unwrap(args.Holder()); + CHECK_NE(wrap, nullptr); + CHECK(wrap->initialized_); - if (wrap == nullptr || wrap->initialized_ == false) - return; wrap->initialized_ = false; - HandleWrap::Close(args); } diff --git a/test/parallel/test-fs-watch-enoent.js b/test/parallel/test-fs-watch-enoent.js index 38f10e1430a360..e6db65993d666e 100644 --- a/test/parallel/test-fs-watch-enoent.js +++ b/test/parallel/test-fs-watch-enoent.js @@ -1,21 +1,64 @@ 'use strict'; + +// This verifies the error thrown by fs.watch. + const common = require('../common'); const assert = require('assert'); const fs = require('fs'); +const tmpdir = require('../common/tmpdir'); +const path = require('path'); +const nonexistentFile = path.join(tmpdir.path, 'non-existent'); +const uv = process.binding('uv'); + +tmpdir.refresh(); + +{ + const validateError = (err) => { + assert.strictEqual(err.path, nonexistentFile); + assert.strictEqual(err.filename, nonexistentFile); + assert.strictEqual(err.syscall, 'watch'); + if (err.code === 'ENOENT') { + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, watch '${nonexistentFile}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + } else { // AIX + assert.strictEqual( + err.message, + `ENODEV: no such device, watch '${nonexistentFile}'`); + assert.strictEqual(err.errno, uv.UV_ENODEV); + assert.strictEqual(err.code, 'ENODEV'); + } + return true; + }; + + assert.throws( + () => fs.watch(nonexistentFile, common.mustNotCall()), + validateError + ); +} + +{ + const file = path.join(tmpdir.path, 'file-to-watch'); + fs.writeFileSync(file, 'test'); + const watcher = fs.watch(file, common.mustNotCall()); + + const validateError = (err) => { + assert.strictEqual(err.path, nonexistentFile); + assert.strictEqual(err.filename, nonexistentFile); + assert.strictEqual( + err.message, + `ENOENT: no such file or directory, watch '${nonexistentFile}'`); + assert.strictEqual(err.errno, uv.UV_ENOENT); + assert.strictEqual(err.code, 'ENOENT'); + assert.strictEqual(err.syscall, 'watch'); + fs.unlinkSync(file); + return true; + }; + + watcher.on('error', common.mustCall(validateError)); -assert.throws(function() { - fs.watch('non-existent-file'); -}, function(err) { - assert(err); - assert(/non-existent-file/.test(err)); - assert.strictEqual(err.filename, 'non-existent-file'); - return true; -}); - -const watcher = fs.watch(__filename); -watcher.on('error', common.mustCall(function(err) { - assert(err); - assert(/non-existent-file/.test(err)); - assert.strictEqual(err.filename, 'non-existent-file'); -})); -watcher._handle.onchange(-1, 'ENOENT', 'non-existent-file'); + // Simulate the invocation from the binding + watcher._handle.onchange(uv.UV_ENOENT, 'ENOENT', nonexistentFile); +} diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js index 7affe370c7ed03..f980363b9fd87e 100644 --- a/test/parallel/test-fs-watch.js +++ b/test/parallel/test-fs-watch.js @@ -65,10 +65,16 @@ for (const testCase of cases) { assert.strictEqual(eventType, 'change'); assert.strictEqual(argFilename, testCase.fileName); - watcher.start(); // should not crash - + common.expectsError(() => watcher.start(), { + code: 'ERR_FS_WATCHER_ALREADY_STARTED', + message: 'The watcher has already been started' + }); // end of test case watcher.close(); + common.expectsError(() => watcher.close(), { + code: 'ERR_FS_WATCHER_NOT_STARTED', + message: 'The watcher has not been started' + }); })); // long content so it's actually flushed. toUpperCase so there's real change. @@ -78,3 +84,15 @@ for (const testCase of cases) { fs.writeFileSync(testCase.filePath, content2); }, 100); } + +[false, 1, {}, [], null, undefined].forEach((i) => { + common.expectsError( + () => fs.watch(i, common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "filename" argument must be one of ' + + 'type string, Buffer, or URL' + } + ); +}); diff --git a/test/sequential/test-fs-watch.js b/test/sequential/test-fs-watch.js index b672e9c75395dd..91d750acd00fe2 100644 --- a/test/sequential/test-fs-watch.js +++ b/test/sequential/test-fs-watch.js @@ -112,12 +112,15 @@ tmpdir.refresh(); // https://github.com/joyent/node/issues/6690 { let oldhandle; - assert.throws(function() { + assert.throws(() => { const w = fs.watch(__filename, common.mustNotCall()); oldhandle = w._handle; w._handle = { close: w._handle.close }; w.close(); - }, /^TypeError: Illegal invocation$/); + }, { + message: 'handle must be a FSEvent', + code: 'ERR_ASSERTION' + }); oldhandle.close(); // clean up assert.throws(function() {