Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs,test: throw rather than assert if condition is possible #32028

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1966,6 +1966,12 @@ from another module.

A string that contained unescaped characters was received.

<a id="ERR_UNEXPECTED_INSTANCE"></a>
### `ERR_UNEXPECTED_INSTANCE`

A value's prototype chain lacks an expected constructor. This could be a result
of monkey-patching gone awry or prototype poisoning.

<a id="ERR_UNHANDLED_ERROR"></a>
### `ERR_UNHANDLED_ERROR`

Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,9 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
'callback was already active',
Error);
E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError);
E('ERR_UNEXPECTED_INSTANCE',
'%s must have %s in its prototype chain',
TypeError);
E('ERR_UNHANDLED_ERROR',
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
Expand Down
9 changes: 6 additions & 3 deletions lib/internal/fs/watchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const {
} = require('internal/async_hooks');
const { toNamespacedPath } = require('path');
const { validateUint32 } = require('internal/validators');
const assert = require('internal/assert');

const kOldStatus = Symbol('kOldStatus');
const kUseBigint = Symbol('kUseBigint');
Expand Down Expand Up @@ -163,7 +162,9 @@ FSWatcher.prototype[kFSWatchStart] = function(filename,
if (this._handle === null) { // closed
return;
}
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it as is. Use of assertions to ensure that requirements are met is a common pattern used by many languages. Also the ERR_UNEXPECTED_INSTANCE custom error does not make sense in my opinion. It's an error that should not exist.

That said I won't block this if others are ok with removing assertions from the code base.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to keep it as is. Use of assertions to ensure that requirements are met is a common pattern used by many languages.

I certainly am not excited about adding another error code--wish we had vastly fewer codes--so could leave the assert() part alone. This PR would then just add the test case and remove the expectInternalAssertion() from common.

if (!(this._handle instanceof FSEvent)) {
throw new errors.codes.ERR_UNEXPECTED_INSTANCE('handle', 'FSEvent');
}
if (this._handle.initialized) { // already started
return;
}
Expand Down Expand Up @@ -199,7 +200,9 @@ FSWatcher.prototype.close = function() {
if (this._handle === null) { // closed
return;
}
assert(this._handle instanceof FSEvent, 'handle must be a FSEvent');
if (!(this._handle instanceof FSEvent)) {
throw new errors.codes.ERR_UNEXPECTED_INSTANCE('handle', 'FSEvent');
}
if (!this._handle.initialized) { // not started
return;
}
Expand Down
14 changes: 0 additions & 14 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -547,19 +547,6 @@ function expectsError(validator, exact) {
}, exact);
}

const suffix = 'This is caused by either a bug in Node.js ' +
'or incorrect usage of Node.js internals.\n' +
'Please open an issue with this stack trace at ' +
'https://github.com/nodejs/node/issues\n';

function expectsInternalAssertion(fn, message) {
assert.throws(fn, {
message: `${message}\n${suffix}`,
name: 'Error',
code: 'ERR_INTERNAL_ASSERTION'
});
}

function skipIfInspectorDisabled() {
if (!process.features.inspector) {
skip('V8 inspector is disabled');
Expand Down Expand Up @@ -680,7 +667,6 @@ const common = {
createZeroFilledFile,
disableCrashOnUnhandledRejection,
expectsError,
expectsInternalAssertion,
expectWarning,
getArrayBufferViews,
getBufferSources,
Expand Down
11 changes: 7 additions & 4 deletions test/parallel/test-internal-errors.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Flags: --expose-internals
'use strict';
const common = require('../common');
require('../common');
const {
hijackStdout,
restoreStdout,
Expand Down Expand Up @@ -50,10 +50,13 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error);
}

{
common.expectsInternalAssertion(
assert.throws(
() => new errors.codes.TEST_ERROR_1(),
'Code: TEST_ERROR_1; The provided arguments ' +
'length (0) does not match the required ones (1).'
{
message: /^Code: TEST_ERROR_1; The provided arguments length \(0\) does not match the required ones \(1\)\./,
name: 'Error',
code: 'ERR_INTERNAL_ASSERTION'
}
);
}

Expand Down
30 changes: 28 additions & 2 deletions test/sequential/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,40 @@ tmpdir.refresh();
// https://github.com/joyent/node/issues/6690
{
let oldhandle;
common.expectsInternalAssertion(
assert.throws(
() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
w._handle = { close: w._handle.close };
w.close();
},
'handle must be a FSEvent'
{
name: 'TypeError',
code: 'ERR_UNEXPECTED_INSTANCE',
message: 'handle must have FSEvent in its prototype chain',
}
);
oldhandle.close(); // clean up
}

{
let oldhandle;
assert.throws(
() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
const protoSymbols =
Object.getOwnPropertySymbols(Object.getPrototypeOf(w));
const kFSWatchStart =
protoSymbols.find((val) => val.toString() === 'Symbol(kFSWatchStart)');
w._handle = {};
w[kFSWatchStart]();
},
{
name: 'TypeError',
code: 'ERR_UNEXPECTED_INSTANCE',
message: 'handle must have FSEvent in its prototype chain',
}
);
oldhandle.close(); // clean up
}