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

dgram: improve "address" parameter behavior in Socket.prototype.send #10473

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@boneskull
Contributor

boneskull commented Dec 27, 2016

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)

dgram

Description of change

When using Socket.prototype.send, I was confused about when address was required, since the docs are ambiguous at best, and conflicting at worst: they claim address will default to 127.0.0.1, but this is only in the case where callback is not supplied.

My confusion manifest in an exception from the dns module, which was unhelpful:

dns.js:112
    throw new TypeError('Invalid arguments: ' +
    ^

TypeError: Invalid arguments: hostname must be a string or falsey
    at Object.lookup (dns.js:112:11)
    at lookup (dgram.js:27:14)
    at UDP.lookup4 [as lookup] (dgram.js:32:10)
    at Socket.send (dgram.js:362:16)

dns.lookup expects a valid hostname argument, yet this parameter name ("hostname") is not used in Socket.prototype.send. I thought it better to fail with helpful error messaging before getting as far as dns.lookup, which prompted this PR.

The signature of Socket.prototype.send is a bit awkward, and perhaps the best fix here is to change it. I see this PR as being an improvement nonetheless. Description of changes follow.

UPDATE Jan 8 17

Upon suggestion by @mscdex, I've modified the changes to make address optional in all cases.

Summary of changes:

  • In Socket.prototype.send, address is now always optional; it is no longer required to use callback. The signature has changed from this:
    socket.send(msg, [offset, length,] port, address [, callback])
    
    to this:
    socket.send(msg, [offset, length,] port, [, address] [, callback])
    
  • Faster failure and more helpful error messaging in case when address argument is present but invalid; exceptions no longer fall through to dns module.
  • Updates docs, adds tests.
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Dec 27, 2016

Contributor

I think it would probably be better to just support the 'callback without address' case instead of adding a note/warning in the documentation.

Contributor

mscdex commented Dec 27, 2016

I think it would probably be better to just support the 'callback without address' case instead of adding a note/warning in the documentation.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Dec 27, 2016

Contributor

@mscdex I agree. I don't see any technical reason why we couldn't. I didn't want to be presumptuous.

I'll update this PR accordingly.

Contributor

boneskull commented Dec 27, 2016

@mscdex I agree. I don't see any technical reason why we couldn't. I didn't want to be presumptuous.

I'll update this PR accordingly.

@boneskull boneskull changed the title from dgram: improve Socket docs and invalid arg behavior to dgram: improve "address" parameter behavior in Socket.prototype.send Jan 9, 2017

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 9, 2017

Contributor

@mscdex I've updated this PR as suggested, rebased, etc. My local tests now pass 100%.

Contributor

boneskull commented Jan 9, 2017

@mscdex I've updated this PR as suggested, rebased, etc. My local tests now pass 100%.

Show outdated Hide outdated lib/dgram.js Outdated
Show outdated Hide outdated lib/dgram.js Outdated
@sam-github

I don't see test cases for combinations of null, undefined, and empty string being passed as addresses, with and without offset+length, and with and without a callback. This is a lot of combinatorics, btw, I've no strong opinions, but perhaps it may be best to write the test as one test file for dgram.send() arg processing?

Show outdated Hide outdated lib/dgram.js Outdated
Show outdated Hide outdated doc/api/dgram.md Outdated
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 11, 2017

Member

Btw, labelled as minor because of

Upon suggestion by @mscdex, I've modified the changes to make address optional in all cases.

but perhaps it should be major because of

more helpful error messaging in case when address argument is present but invalid; exceptions no longer fall through to dns module.

And of course if it starts rejecting previously valid input, it must be major, but that's still under discussion, I think.

Member

sam-github commented Jan 11, 2017

Btw, labelled as minor because of

Upon suggestion by @mscdex, I've modified the changes to make address optional in all cases.

but perhaps it should be major because of

more helpful error messaging in case when address argument is present but invalid; exceptions no longer fall through to dns module.

And of course if it starts rejecting previously valid input, it must be major, but that's still under discussion, I think.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

more helpful error messaging in case when address argument is present but invalid; exceptions no longer fall through to dns module.
And of course if it starts rejecting previously valid input, it must be major, but that's still under discussion, I think.

It doesn't. Either would throw an error; the errors thrown in this PR are more helpful/obvious.

UPDATE By "fall through" I mean the dns module currently throws some errors here. My changes throw errors in the same situations (in a more helpful manner) before getting as far as the dns module.

Contributor

boneskull commented Jan 11, 2017

more helpful error messaging in case when address argument is present but invalid; exceptions no longer fall through to dns module.
And of course if it starts rejecting previously valid input, it must be major, but that's still under discussion, I think.

It doesn't. Either would throw an error; the errors thrown in this PR are more helpful/obvious.

UPDATE By "fall through" I mean the dns module currently throws some errors here. My changes throw errors in the same situations (in a more helpful manner) before getting as far as the dns module.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 11, 2017

Member

@boneskull Changes in error message text are considered semver-major, because they can break apps that match against err.message. Its an API breakage, but not controversial, better error messages are a good thing. So, sounds like this is semver-major?

Member

sam-github commented Jan 11, 2017

@boneskull Changes in error message text are considered semver-major, because they can break apps that match against err.message. Its an API breakage, but not controversial, better error messages are a good thing. So, sounds like this is semver-major?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 11, 2017

Member

@boneskull FYI, I would have done this as one commit introducing tests, because the test coverage wasn't there over the invalid addresses, asserting the current behaviour, then a follow-on commit that improves the error messages. That would as a side-effect also make whether it is an API change or not crystal clear - because changes to test behaviour are API changes. Its probably too much work to rebase it like that, but a 2-phase approach for code that has no test coverage ATM is easier to review.

Member

sam-github commented Jan 11, 2017

@boneskull FYI, I would have done this as one commit introducing tests, because the test coverage wasn't there over the invalid addresses, asserting the current behaviour, then a follow-on commit that improves the error messages. That would as a side-effect also make whether it is an API change or not crystal clear - because changes to test behaviour are API changes. Its probably too much work to rebase it like that, but a 2-phase approach for code that has no test coverage ATM is easier to review.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

@boneskull Changes in error message text are considered semver-major, because they can break apps that match against err.message. Its an API breakage, but not controversial, better error messages are a good thing. So, sounds like this is semver-major?

That's an unfortunate--but likely necessary--policy.

I want to make it clear that I'm not interested in expanding this PR to further restrict allowed types or values of the address parameter (e.g., disallowing false). If we go down that road, the same logic suggests we should also change dns.lookup(). Basically, this would be much more effort than I had intended to give this PR.

Contributor

boneskull commented Jan 11, 2017

@boneskull Changes in error message text are considered semver-major, because they can break apps that match against err.message. Its an API breakage, but not controversial, better error messages are a good thing. So, sounds like this is semver-major?

That's an unfortunate--but likely necessary--policy.

I want to make it clear that I'm not interested in expanding this PR to further restrict allowed types or values of the address parameter (e.g., disallowing false). If we go down that road, the same logic suggests we should also change dns.lookup(). Basically, this would be much more effort than I had intended to give this PR.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

Its probably too much work to rebase it like that, but a 2-phase approach for code that has no test coverage ATM is easier to review.

This is actually pretty straightforward to do.

Contributor

boneskull commented Jan 11, 2017

Its probably too much work to rebase it like that, but a 2-phase approach for code that has no test coverage ATM is easier to review.

This is actually pretty straightforward to do.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 11, 2017

Member

I understand. This PR doesn't change the behaviour, other than making the error messages much more helpful.

The tests and docs do reveal that the current behaviour is not ideal, and now that you have done this work, its easier to discuss how we wish the API would work, and change it in the future.

Member

sam-github commented Jan 11, 2017

I understand. This PR doesn't change the behaviour, other than making the error messages much more helpful.

The tests and docs do reveal that the current behaviour is not ideal, and now that you have done this work, its easier to discuss how we wish the API would work, and change it in the future.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

I understand. This PR doesn't change the behaviour, other than making the error messages much more helpful.

That's not entirely true either. This PR also allows callback without address. It changes the API of Socket.prototype.send() in a non-breaking manner--i.e., it adds w/o taking away.

// previous to this PR, the following would throw
socket.send(buf, port, () => {
  console.log('done');
});
Contributor

boneskull commented Jan 11, 2017

I understand. This PR doesn't change the behaviour, other than making the error messages much more helpful.

That's not entirely true either. This PR also allows callback without address. It changes the API of Socket.prototype.send() in a non-breaking manner--i.e., it adds w/o taking away.

// previous to this PR, the following would throw
socket.send(buf, port, () => {
  console.log('done');
});
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

The tests and docs do reveal that the current behaviour is not ideal, and now that you have done this work, its easier to discuss how we wish the API would work, and change it in the future.

Yes. This is silly, but would work:

socket.send(buf, port, '', () => {
  console.log('address was 127.0.0.1');
});

What's sillier is that currently you'd have to write something like the above code if you wanted a callback and didn't care to specify the address. 😝

Contributor

boneskull commented Jan 11, 2017

The tests and docs do reveal that the current behaviour is not ideal, and now that you have done this work, its easier to discuss how we wish the API would work, and change it in the future.

Yes. This is silly, but would work:

socket.send(buf, port, '', () => {
  console.log('address was 127.0.0.1');
});

What's sillier is that currently you'd have to write something like the above code if you wanted a callback and didn't care to specify the address. 😝

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 11, 2017

Contributor

To recap, my plan is:

  • Change some verbiage in docs around "specify"
  • Ensure new error messaging is consistent with core as a whole
  • Split PR into two commits:
    • tests against currently uncovered & undocumented behavior
    • implementation of and more tests around new behavior
Contributor

boneskull commented Jan 11, 2017

To recap, my plan is:

  • Change some verbiage in docs around "specify"
  • Ensure new error messaging is consistent with core as a whole
  • Split PR into two commits:
    • tests against currently uncovered & undocumented behavior
    • implementation of and more tests around new behavior
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 13, 2017

Contributor

@mscdex @sam-github Please take another gander.

Contributor

boneskull commented Jan 13, 2017

@mscdex @sam-github Please take another gander.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 13, 2017

Contributor

I also realized this squashes a particularly gross bug--if send() was called with an invalid address type, but the socket was not yet fully bound, the send would be enqueued. Once binding was complete, the send would run, and the exception from dns.lookup() would be thrown asynchronously.

You can see this problem illustrated in the tests of my first commit. I was not able to assert the exceptions were thrown unless I did this (or caught them at the process level). I removed the up-front binding from this test file in the second commit, since my changes fix the bug.

Contributor

boneskull commented Jan 13, 2017

I also realized this squashes a particularly gross bug--if send() was called with an invalid address type, but the socket was not yet fully bound, the send would be enqueued. Once binding was complete, the send would run, and the exception from dns.lookup() would be thrown asynchronously.

You can see this problem illustrated in the tests of my first commit. I was not able to assert the exceptions were thrown unless I did this (or caught them at the process level). I removed the up-front binding from this test file in the second commit, since my changes fix the bug.

@sam-github

Two small nits, but otherwise LGTM

Show outdated Hide outdated doc/api/dgram.md Outdated
Show outdated Hide outdated test/parallel/test-dgram-send-callback-buffer-length-empty-address.js Outdated
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Jan 13, 2017

Contributor

rebased onto master + linted

Contributor

boneskull commented Jan 13, 2017

rebased onto master + linted

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 9, 2017

Contributor

Trying again with 61fdbd8.

Contributor

boneskull commented Feb 9, 2017

Trying again with 61fdbd8.

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Feb 9, 2017

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 9, 2017

Contributor

arm failure again is unrelated

@cjihrig would appreciate a 👍

😄

Contributor

boneskull commented Feb 9, 2017

arm failure again is unrelated

@cjihrig would appreciate a 👍

😄

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 10, 2017

Contributor

conflict resolved

Contributor

boneskull commented Feb 10, 2017

conflict resolved

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 11, 2017

Member

I'm trying to run CI against this PR and it is not going well. I'm pretty sure the problem is with CI and not this PR, though. Maybe someone else seeing this in another hour or two can try again?

Member

Trott commented Feb 11, 2017

I'm trying to run CI against this PR and it is not going well. I'm pretty sure the problem is with CI and not this PR, though. Maybe someone else seeing this in another hour or two can try again?

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 12, 2017

Contributor

@Trott Anything I can do to help?

Contributor

boneskull commented Feb 12, 2017

@Trott Anything I can do to help?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 14, 2017

Member

@boneskull Yes! Can you remove the merge commit? Rumor has it that is the source of the issue.

Member

Trott commented Feb 14, 2017

@boneskull Yes! Can you remove the merge commit? Rumor has it that is the source of the issue.

@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 14, 2017

Contributor

@Trott Hm, that's what I get for using GitHub's new conflict resolver tool, I guess. 😄

Contributor

boneskull commented Feb 14, 2017

@Trott Hm, that's what I get for using GitHub's new conflict resolver tool, I guess. 😄

dgram: add more tests for Socket.prototype.send
- Add coverage around valid, undocumented types for `address` parameter.
- Add coverage around known invalid, but uncovered, types for `address`
  parameter.
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 14, 2017

Contributor

@Trott rebased

Contributor

boneskull commented Feb 14, 2017

@Trott rebased

@Trott

This comment has been minimized.

Show comment
Hide comment
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 14, 2017

Member

This last round of changes seem like nitpicks (for lack of a better word), in my opinion. You may feel differently, but the relative impact on the PR as a whole is minimal.

@boneskull This is fair, but not being strict about things like #10473 (comment) have caused breakage in the past.

I think you're right though, what we need is to have all this stuff covered by linter rules, so that everything can all be fixed at once.


Speaking of which, regarding this (by @cjihrig):

Instead of just checking for the TypeError constructor, we've been moving toward a check like this:

/^TypeError: Invalid arguments: address must be a nonempty string or falsy$/

You can put it in a variable and reuse it across these assertions.

This seems like something we should lint for in test/, i.e. we should require the second parameter in assert.throws() to be a regex starting with ^ and ending with $. (@not-an-aardvark @Trott @silverwind @targos)

Member

gibfahn commented Feb 14, 2017

This last round of changes seem like nitpicks (for lack of a better word), in my opinion. You may feel differently, but the relative impact on the PR as a whole is minimal.

@boneskull This is fair, but not being strict about things like #10473 (comment) have caused breakage in the past.

I think you're right though, what we need is to have all this stuff covered by linter rules, so that everything can all be fixed at once.


Speaking of which, regarding this (by @cjihrig):

Instead of just checking for the TypeError constructor, we've been moving toward a check like this:

/^TypeError: Invalid arguments: address must be a nonempty string or falsy$/

You can put it in a variable and reuse it across these assertions.

This seems like something we should lint for in test/, i.e. we should require the second parameter in assert.throws() to be a regex starting with ^ and ending with $. (@not-an-aardvark @Trott @silverwind @targos)

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 14, 2017

Member

i.e. we should require the second parameter in assert.throws() to be a regex starting with ^ and ending with $.

We should definitely allow validation functions as an alternative too. It's just constructors that can be problematic.

There's more to talk about regarding this (specifically if we might be moving towards a situation where error message changes are not breaking changes anymore thanks to the error identifiers that @jasnell has introduced, and if so then whether that means fully-matching regexps will become anti-patterns as the second argument). But this is not the place for that discussion. (Personally, I think we're probably a long way off from being able to treat message changes as non-breaking changes, but I'm often wrong.)

Member

Trott commented Feb 14, 2017

i.e. we should require the second parameter in assert.throws() to be a regex starting with ^ and ending with $.

We should definitely allow validation functions as an alternative too. It's just constructors that can be problematic.

There's more to talk about regarding this (specifically if we might be moving towards a situation where error message changes are not breaking changes anymore thanks to the error identifiers that @jasnell has introduced, and if so then whether that means fully-matching regexps will become anti-patterns as the second argument). But this is not the place for that discussion. (Personally, I think we're probably a long way off from being able to treat message changes as non-breaking changes, but I'm often wrong.)

@not-an-aardvark

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Feb 14, 2017

Member

One potential concern about linting for error constructors is that the rule might report an error for this code:

var ERROR_PATTERN = /^Error: Something bad happened$/;

// later...

// as far as the rule is concerned, ERROR_PATTERN could be a constructor
assert.throws(foo, ERROR_PATTERN);
assert.throws(bar, ERROR_PATTERN);

It would be relatively simple to check for constructors that are known beforehand, e.g. TypeError.

Member

not-an-aardvark commented Feb 14, 2017

One potential concern about linting for error constructors is that the rule might report an error for this code:

var ERROR_PATTERN = /^Error: Something bad happened$/;

// later...

// as far as the rule is concerned, ERROR_PATTERN could be a constructor
assert.throws(foo, ERROR_PATTERN);
assert.throws(bar, ERROR_PATTERN);

It would be relatively simple to check for constructors that are known beforehand, e.g. TypeError.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 14, 2017

Member

/cc @nodejs/ctc

This semver-major has sufficient approvals to land, but it's been a sufficiently long time coming that it might be off the radar for some folks. PTAL if you get a chance.

Member

Trott commented Feb 14, 2017

/cc @nodejs/ctc

This semver-major has sufficient approvals to land, but it's been a sufficiently long time coming that it might be off the radar for some folks. PTAL if you get a chance.

const client = dgram.createSocket('udp4');
const messageSent = common.mustCall(function messageSent(err, bytes) {

This comment has been minimized.

@thefourtheye

thefourtheye Feb 14, 2017

Contributor

assert.ifError(err)

@thefourtheye

thefourtheye Feb 14, 2017

Contributor

assert.ifError(err)

Show outdated Hide outdated test/parallel/test-dgram-send-callback-multi-buffer-empty-address.js Outdated
const offset = 20;
const len = buf.length - offset;
const onMessage = common.mustCall(function messageSent(err, bytes) {

This comment has been minimized.

@thefourtheye

thefourtheye Feb 14, 2017

Contributor

err has to be asserted first.

@thefourtheye

thefourtheye Feb 14, 2017

Contributor

err has to be asserted first.

This comment has been minimized.

@boneskull

boneskull Feb 14, 2017

Contributor

FWIW, these two files are copypasta from test-dgram-send-callback-buffer-length (etc) (some of) which have the same issues.

@boneskull

boneskull Feb 14, 2017

Contributor

FWIW, these two files are copypasta from test-dgram-send-callback-buffer-length (etc) (some of) which have the same issues.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 14, 2017

Member

OMG the test/arm results display is fixed! :-D

Member

Trott commented Feb 14, 2017

OMG the test/arm results display is fixed! :-D

dgram: Improve signature of Socket.prototype.send
- Do not require presence of `address` parameter to use `callback`
  parameter; `address` is *always* optional
- Improve exception messaging if `address` is invalid type
- If `address` is an invalid type, guarantee a synchronously thrown
  exception
- Update documentation to reflect signature changes
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 14, 2017

Contributor

@thefourtheye Changes applied

Contributor

boneskull commented Feb 14, 2017

@thefourtheye Changes applied

@Trott

This comment has been minimized.

Show comment
Hide comment
@boneskull

This comment has been minimized.

Show comment
Hide comment
@boneskull

boneskull Feb 17, 2017

Contributor

ping anyone

Contributor

boneskull commented Feb 17, 2017

ping anyone

client.send(buf, port, true);
}, expectedError);
client.unref();

This comment has been minimized.

@cjihrig

cjihrig Feb 17, 2017

Contributor

I just noticed this. Is there any chance that the socket would be unref'ed and the process exit before all six messages are received. I am able to make the test fail artificially by adding a short timeout around the last successful send. It doesn't seem to be a problem on the CI, but wouldn't want it to be a source of future flakiness either.

@cjihrig

cjihrig Feb 17, 2017

Contributor

I just noticed this. Is there any chance that the socket would be unref'ed and the process exit before all six messages are received. I am able to make the test fail artificially by adding a short timeout around the last successful send. It doesn't seem to be a problem on the CI, but wouldn't want it to be a source of future flakiness either.

Trott added a commit to Trott/io.js that referenced this pull request Feb 17, 2017

dgram: improve signature of Socket.prototype.send
- Do not require presence of `address` parameter to use `callback`
  parameter; `address` is *always* optional
- Improve exception messaging if `address` is invalid type
- If `address` is an invalid type, guarantee a synchronously thrown
  exception
- Update documentation to reflect signature changes
- Add coverage around valid, undocumented types for `address` parameter.
- Add coverage around known invalid, but uncovered, types for `address`
  parameter.

PR-URL: nodejs#10473
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 17, 2017

Member

Landed in 32679c7.
Thanks for the contribution! 🎉

Member

Trott commented Feb 17, 2017

Landed in 32679c7.
Thanks for the contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment