Skip to content

Commit

Permalink
lib: throw a special error in internal/assert
Browse files Browse the repository at this point in the history
Instead of using the public AssertionError, use a simplified
error that describes potential causes of these assertions
and suggests the user to open an issue.

PR-URL: #26635
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
joyeecheung authored and BridgeAR committed Apr 24, 2019
1 parent 237d9f9 commit b9f1e57
Show file tree
Hide file tree
Showing 12 changed files with 100 additions and 28 deletions.
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,12 @@ is set for the `Http2Stream`.
`http2.connect()` was passed a URL that uses any protocol other than `http:` or
`https:`.

<a id="ERR_INTERNAL_ASSERTION"></a>
### ERR_INTERNAL_ASSERTION

There was a bug in Node.js or incorrect usage of Node.js internals.
To fix the error, open an issue at https://github.com/nodejs/node/issues.

<a id="ERR_INCOMPATIBLE_OPTION_PAIR"></a>
### ERR_INCOMPATIBLE_OPTION_PAIR

Expand Down
13 changes: 11 additions & 2 deletions lib/internal/assert.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
'use strict';

let error;
function lazyError() {
if (!error) {
error = require('internal/errors').codes.ERR_INTERNAL_ASSERTION;
}
return error;
}
function assert(value, message) {
if (!value) {
require('assert')(value, message);
const ERR_INTERNAL_ASSERTION = lazyError();
throw new ERR_INTERNAL_ASSERTION(message);
}
}

function fail(message) {
require('assert').fail(message);
const ERR_INTERNAL_ASSERTION = lazyError();
throw new ERR_INTERNAL_ASSERTION(message);
}

assert.fail = fail;
Expand Down
7 changes: 7 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,13 @@ E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error);
E('ERR_INSPECTOR_COMMAND', 'Inspector error %d: %s', Error);
E('ERR_INSPECTOR_NOT_AVAILABLE', 'Inspector is not available', Error);
E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error);
E('ERR_INTERNAL_ASSERTION', (message) => {
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';
return message === undefined ? suffix : `${message}\n${suffix}`;
}, Error);
E('ERR_INVALID_ADDRESS_FAMILY', function(addressType, host, port) {
this.host = host;
this.port = port;
Expand Down
14 changes: 14 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,19 @@ function expectsError(fn, settings, exact) {
return mustCall(innerFn, 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 @@ -729,6 +742,7 @@ module.exports = {
enoughTestCpu,
enoughTestMem,
expectsError,
expectsInternalAssertion,
expectWarning,
getArrayBufferViews,
getBufferSources,
Expand Down
7 changes: 7 additions & 0 deletions test/message/internal_assert.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

// Flags: --expose-internals
require('../common');

const assert = require('internal/assert');
assert(false);
15 changes: 15 additions & 0 deletions test/message/internal_assert.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
internal/assert.js:*
throw new ERR_INTERNAL_ASSERTION(message);
^

Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

at assert (internal/assert.js:*:*)
at * (*test*message*internal_assert.js:7:1)
at *
at *
at *
at *
at *
at *
7 changes: 7 additions & 0 deletions test/message/internal_assert_fail.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

// Flags: --expose-internals
require('../common');

const assert = require('internal/assert');
assert.fail('Unreachable!');
16 changes: 16 additions & 0 deletions test/message/internal_assert_fail.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
internal/assert.js:*
throw new ERR_INTERNAL_ASSERTION(message);
^

Error [ERR_INTERNAL_ASSERTION]: Unreachable!
This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

at Function.fail (internal/assert.js:*:*)
at * (*test*message*internal_assert_fail.js:7:8)
at *
at *
at *
at *
at *
at *
10 changes: 3 additions & 7 deletions test/parallel/test-internal-assert.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
// Flags: --expose-internals
'use strict';

// This tests that the internal assert module works as expected.
// The failures are tested in test/message.

require('../common');

const assert = require('assert');
const internalAssert = require('internal/assert');

// Should not throw.
internalAssert(true);
internalAssert(true, 'fhqwhgads');

assert.throws(() => { internalAssert(false); }, assert.AssertionError);
assert.throws(() => { internalAssert(false, 'fhqwhgads'); },
{ code: 'ERR_ASSERTION', message: 'fhqwhgads' });
assert.throws(() => { internalAssert.fail('fhqwhgads'); },
{ code: 'ERR_ASSERTION', message: 'fhqwhgads' });
8 changes: 3 additions & 5 deletions test/parallel/test-internal-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,10 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error);
}

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

Expand Down
7 changes: 2 additions & 5 deletions test/parallel/test-tls-basic-validations.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,9 @@ common.expectsError(
assert.throws(() => tls.createServer({ ticketKeys: Buffer.alloc(0) }),
/TypeError: Ticket keys length must be 48 bytes/);

common.expectsError(
common.expectsInternalAssertion(
() => tls.createSecurePair({}),
{
code: 'ERR_ASSERTION',
message: 'context.context must be a NativeSecureContext'
}
'context.context must be a NativeSecureContext'
);

{
Expand Down
18 changes: 9 additions & 9 deletions test/sequential/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,14 @@ tmpdir.refresh();
// https://github.com/joyent/node/issues/6690
{
let oldhandle;
assert.throws(() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
w._handle = { close: w._handle.close };
w.close();
}, {
message: 'handle must be a FSEvent',
code: 'ERR_ASSERTION'
});
common.expectsInternalAssertion(
() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
w._handle = { close: w._handle.close };
w.close();
},
'handle must be a FSEvent'
);
oldhandle.close(); // clean up
}

0 comments on commit b9f1e57

Please sign in to comment.