1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fs: improve errors thrown from fs.watch()

Browse Source 
- Add an accessor property `initialized `to FSEventWrap to
  check the state of the handle from the JS land
- Introduce ERR_FS_WATCHER_ALREADY_STARTED so calling start()
  on a watcher that is already started will throw instead of
  doing nothing silently.
- Introduce ERR_FS_WATCHER_NOT_STARTED so calling close()
  on a watcher that is already closed will throw instead of
  doing nothing silently.
- Validate the filename passed to fs.watch()
- Assert that the handle in the watcher are instances of
  FSEvent instead of relying on the illegal invocation error
  from the VM.
- Add more assertions in FSEventWrap methods now that we check
  `initialized` and the filename in JS land before invoking
  the binding.
- Use uvException instead of errornoException to create
  the errors with the error numbers from libuv to make them
  consistent with other errors in fs.

TODO:

- Improve fs.watchFile() the same way this patch improves fs.watch()
- It seems possible to fire both rename and change event from libuv
  together now that we can check if the handle is closed via
  `initialized` in JS land.

PR-URL: https://github.com/nodejs/node/pull/19089
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This commit is contained in:
Joyee Cheung 2018-03-03 00:28:05 +08:00
parent 48b5c11b16
commit 6c25f2ea49
No known key found for this signature in database
GPG Key ID: F586868AAD831D0C
7 changed files with 178 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
doc/api/errors.md
View File

@ -783,6 +783,18 @@ falsy value.
An invalid symlink type was passed to the [`fs.symlink()`][] or
[`fs.symlinkSync()`][] methods.
<a id="ERR_FS_WATCHER_ALREADY_STARTED"></a>
### ERR_FS_WATCHER_ALREADY_STARTED
An attempt was made to start a watcher returned by `fs.watch()` that has
already been started.
<a id="ERR_FS_WATCHER_NOT_STARTED"></a>
### 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.
<a id="ERR_HTTP_HEADERS_SENT"></a>
### ERR_HTTP_HEADERS_SENT

41
lib/fs.js
View File

@ -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();
};

4
lib/internal/errors.js
View File

@ -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',

65
src/fs_event_wrap.cc
View File

@ -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<Value>& args);
static void Start(const FunctionCallbackInfo<Value>& args);
static void Close(const FunctionCallbackInfo<Value>& args);
static void GetInitialized(const FunctionCallbackInfo<Value>& 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<Value>& args) {
FSEventWrap* wrap = Unwrap<FSEventWrap>(args.This());
CHECK(wrap != nullptr);
args.GetReturnValue().Set(wrap->initialized_);
}
void FSEventWrap::Initialize(Local<Object> target,
Local<Value> unused,
@ -95,6 +105,18 @@ void FSEventWrap::Initialize(Local<Object> target,
env->SetProtoMethod(t, "start", Start);
env->SetProtoMethod(t, "close", Close);
Local<FunctionTemplate> 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<FunctionTemplate>(),
static_cast<PropertyAttribute>(ReadOnly | DontDelete | v8::DontEnum));
target->Set(fsevent_string, t->GetFunction());
}
@ -105,22 +127,19 @@ void FSEventWrap::New(const FunctionCallbackInfo<Value>& args) {
new FSEventWrap(env, args.This());
}
// wrap.start(filename, persistent, recursive, encoding)
void FSEventWrap::Start(const FunctionCallbackInfo<Value>& 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<FSEventWrap>(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,20 +148,22 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& 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);
wrap->initialized_ = true;
if (err != 0) {
FSEventWrap::Close(args);
return args.GetReturnValue().Set(err);
}
if (err == 0) {
// Check for persistent argument
if (!args[1]->IsTrue()) {
uv_unref(reinterpret_cast<uv_handle_t*>(&wrap->handle_));
}
} else {
FSEventWrap::Close(args);
}
}
args.GetReturnValue().Set(err);
}
@ -209,13 +230,11 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
void FSEventWrap::Close(const FunctionCallbackInfo<Value>& args) {
FSEventWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder());
CHECK_NE(wrap, nullptr);
CHECK(wrap->initialized_);
if (wrap == nullptr || wrap->initialized_ == false)
return;
wrap->initialized_ = false;
HandleWrap::Close(args);
}

71
test/parallel/test-fs-watch-enoent.js
View File

@ -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');
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');
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;
});
};
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');
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));
// Simulate the invocation from the binding
watcher._handle.onchange(uv.UV_ENOENT, 'ENOENT', nonexistentFile);
}

22
test/parallel/test-fs-watch.js
View File

@ -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'
}
);
});

7
test/sequential/test-fs-watch.js
View File

@ -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() {