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.

Sign up for GitHub

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

Jump to bottom

net: migrate errors to internal/errors #17766

Closed
wants to merge 7 commits into from
Closed

net: migrate errors to internal/errors #17766

wants to merge 7 commits into from

Conversation

kysnm
Copy link
Contributor

@kysnm kysnm commented Dec 19, 2017 •
edited
Loading

Refs: #17709

cc @jasnell @joyeecheung

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, errors, net

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Dec 19, 2017
Trott
Trott reviewed Dec 19, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi @kysnm! Thanks for the pull request! I have no comments on the code changes and will leave that to others, but I have two small nit-pick comments on the docs.

doc/api/errors.md Outdated
### ERR_SERVER_NOT_RUNNING

The [`server.close()`][] method was called when a `net.Server` was not running. This applies to all instances of `net.Server`, including HTTP, HTTPS,
and HTTP/2 Server instances.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "Server" should probably be either in backticks or else lowercase. (Either it is referring to a server generically, in which case lowercase, or it is referring to the Server object, in which case backticks.)

doc/api/errors.md Outdated
<a id="ERR_SERVER_NOT_RUNNING"></a>
### ERR_SERVER_NOT_RUNNING

The [`server.close()`][] method was called when a `net.Server` was not running. This applies to all instances of `net.Server`, including HTTP, HTTPS,
Copy link
Member

Choose a reason for hiding this comment

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

Wrap at 80 chars?

@kysnm kysnm force-pushed the net-migrate-to-internal-errors branch from 086f5f9 to 4f79622 Compare December 20, 2017 02:24
@kysnm
Copy link
Contributor Author

kysnm commented Dec 20, 2017

@Trott
Thank you for review. I have fixed it.

joyeecheung
joyeecheung reviewed Dec 20, 2017
View reviewed changes
test/parallel/test-http-unix-socket.js Outdated
@@ -59,7 +59,7 @@ server.listen(common.PIPE, common.mustCall(function() {
server.close(common.mustCall(function(error) {
assert.strictEqual(error, undefined);
server.close(common.mustCall(function(error) {
assert.strictEqual(error && error.message, 'Not running');
assert.strictEqual(error && error.code, 'ERR_SERVER_NOT_RUNNING');
Copy link
Member

Choose a reason for hiding this comment

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

Can you use common.expectsError() to test this? This should be something like

common.expectsError({
  code: 'ERR_SERVER_NOT_RUNNING',
  message: 'Server is not running.',
  type: Error
})(error);

test/parallel/test-net-socket-destroy-send.js Outdated
@@ -13,10 +13,10 @@ server.listen(0, common.mustCall(function() {
// Test destroy returns this, even on multiple calls when it short-circuits.
assert.strictEqual(conn, conn.destroy().destroy());
conn.on('error', common.mustCall(function(err) {
assert.strictEqual(err.message, 'This socket is closed');
assert.strictEqual(err.code, 'ERR_SOCKET_CLOSED');
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, please use common.expectsError

test/parallel/test-net-socket-write-after-close.js Outdated
@@ -28,8 +28,7 @@ const net = require('net');
const client = net.connect({ port }, common.mustCall(() => {
client.on('error', common.mustCall((err) => {
server.close();
assert.strictEqual(err.constructor, Error);
assert.strictEqual(err.message, 'This socket is closed');
assert.strictEqual(err.code, 'ERR_SOCKET_CLOSED');
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@joyeecheung joyeecheung mentioned this pull request Dec 20, 2017
Closed
6 tasks
@kysnm
Copy link
Contributor Author

kysnm commented Dec 20, 2017

@joyeecheung
Thanks for your review. I have fixed it.

apapirovski
apapirovski reviewed Dec 20, 2017
View reviewed changes
test/parallel/test-net-socket-destroy-send.js Outdated
@@ -13,10 +13,18 @@ server.listen(0, common.mustCall(function() {
// Test destroy returns this, even on multiple calls when it short-circuits.
assert.strictEqual(conn, conn.destroy().destroy());
conn.on('error', common.mustCall(function(err) {
Copy link
Member

Choose a reason for hiding this comment

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

The common.mustCall is not necessary here. common.expectsError creates a function that can receive an error and also makes sure that it actually ran.

apapirovski
apapirovski reviewed Dec 20, 2017
View reviewed changes
test/parallel/test-net-socket-destroy-send.js Outdated
}));
conn.write(Buffer.from('kaboom'), common.mustCall(function(err) {
assert.strictEqual(err.message, 'This socket is closed');
common.expectsError({
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@kysnm
Copy link
Contributor Author

kysnm commented Dec 20, 2017

@apapirovski
Thanks for your review. I have fixed it.

jasnell
jasnell reviewed Dec 20, 2017
View reviewed changes
test/parallel/test-http-unix-socket.js Outdated
code: 'ERR_SERVER_NOT_RUNNING',
message: 'Server is not running.',
type: Error
})(error);
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to...

server.close(common.expectsError({
  code: 'ERR_SERVER_NOT_RUNNING',
  message: 'Server is not running.',
  type: Error
}));

@kysnm
Copy link
Contributor Author

kysnm commented Dec 21, 2017

@jasnell
Sorry for bothering you…

@jasnell
Copy link
Member

jasnell commented Dec 22, 2017

@kysnm .. there's no bother at all :-) thank you for doing this!

@maclover7 maclover7 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 22, 2017
@maclover7
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/12270/

@joyeecheung
Copy link
Member

There were some seemingly unrelated errors in the last CI. New CI: https://ci.nodejs.org/job/node-test-pull-request/12281/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2017
@joyeecheung
Copy link
Member

Landed in b98aaa3, thanks!

@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2017
joyeecheung pushed a commit that referenced this pull request Dec 23, 2017
@kysnm @joyeecheung
net: migrate errors to internal/errors
b98aaa3 
Throw ERR_SOCKET_CLOSED and ERR_SERVER_NOT_RUNNING
instead of the old-style errors in net.js.

PR-URL: #17766
Refs: #17709
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@apapirovski apapirovski apapirovski left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@kysnm @jasnell @maclover7 @joyeecheung @apapirovski @Trott @lpinca @targos @nodejs-github-bot