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

net: give better error messages #35

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Add address and/or port to errors where applicable for better reporting.

See nodejs/node-v0.x-archive#7005 and #16

@@ -0,0 +1,46 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

uhhhhh

@evanlucas
Copy link
Contributor Author

@jonathanong Good point. Removed.

@MauriceButler
Copy link
Contributor

Thanks @evanlucas That 1 less thing I have to do this weekend. 😄

I had a brief look yesterday and there were other areas that used this same error logic, Socket being one of them.

Have you had a look through there as well?

@evanlucas
Copy link
Contributor Author

I have not. I would be more than happy to though.

@rvagg
Copy link
Member

rvagg commented Dec 3, 2014

It's unfortunate that we don't have a solid perf suite yet to assess these kinds of changes!

@evanlucas
Copy link
Contributor Author

For sure. That was a concern when I had opened a PR on the node repo.

var ex = errnoException(err, 'listen');
var details, additions;
if (port > 0) {
details = util.format('%s:%s', address, port);
Copy link
Member

Choose a reason for hiding this comment

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

I think this whole chunk could be moved to a function.

@indutny
Copy link
Member

indutny commented Dec 3, 2014

@evanlucas looks really promising! Left some comments.

@rvagg I'm not sure how this even could be benchmarked, and what would these numbers mean. But technically it should be possible.

@evanlucas do you think you may create some benchmark that will simulate EPIPE or whatever in a large quantities?

@evanlucas
Copy link
Contributor Author

Let me see what I can come up with on the benchmark

@@ -755,7 +755,8 @@ function afterWrite(status, handle, req, err) {
}

if (status < 0) {
var ex = errnoException(status, 'write', err);
err = util.format('%s %s:%s', err, req.address, req.port);
Copy link
Member

Choose a reason for hiding this comment

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

Using simple string concatenation here should be significantly faster than calling util.format(). I/O errors are the exceptional case, of course, but in a large scale applications, they probably happen frequently enough to merit optimizing for.

@evanlucas
Copy link
Contributor Author

Ok, I broke it out into a function and am now setting the properties in code. The syscall is now only called in the event an error actually occurs and I went ahead and fixed up the tests as requested. As far as the benchmark goes, would that need to go in the benchmark directory? I have not been able to run the current net benchmark tests...they just hang, so wasn't sure exactly where you wanted me to put the new benchmark test. Thanks!

@rvagg rvagg force-pushed the v0.12 branch 4 times, most recently from d7e65ff to 185d11c Compare December 4, 2014 10:21
@@ -817,28 +836,46 @@ function connect(self, address, port, addressType, localAddress, localPort) {
err = bind(localAddress, localPort);

if (err) {
self._destroy(errnoException(err, 'bind'));
var ex = detailedException(err, 'bind', address, port);
Copy link

Choose a reason for hiding this comment

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

Did you mean localAddress and localPort because of bind(localAddress, localPort)? You're not passing address and port variables to that call.

@evanlucas
Copy link
Contributor Author

Ok, went ahead and updated those tests. I also fixed the error message to include the local port and address on bind error as pointed out by @xaka. Thanks!

@bnoordhuis
Copy link
Member

LGTM. Can you squash it into a single commit with a nice commit log? If you haven't seen it, CONTRIBUTING.md has an example of a good commit log.

Can I get one more LGTM from another committer? Thanks.

Add address and/or port to errors where applicable for better reporting.
In the event the local address and port are accessible, it will also add
those to the error message.

See nodejs/node-v0.x-archive#7005
@evanlucas
Copy link
Contributor Author

I squashed it into one commit with a better commit log. Thanks

This was referenced Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants