src: use UV_EINVAL instead of EINVAL in udp_wrap #15444

danbev · 2017-09-17T10:21:05Z

Currently if the buffer size exceeds the maximum int size the following
error will be thrown:

dgram.js:180
    throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e);
    ^

Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error:
Unknown system error 22: Unknown system error 22, uv_recv_buffer_size
    at bufferSize (dgram.js:180:11)

It looks like perhaps UV_EINVAL was intended to give the following error
message instead:

dgram.js:180
    throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e);
    ^

Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error:
EINVAL: invalid argument, uv_recv_buffer_size
    at bufferSize (dgram.js:180:11)

This commit changes EINVAL to use UV_EINVAL.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

cjihrig

LGTM. A test would be nice if possible.

danbev · 2017-09-17T16:55:13Z

LGTM. A test would be nice if possible.

Good point, added a test for this.

tniessen · 2017-09-17T19:23:05Z

test/parallel/test-dgram-socket-buffer-size.js

+    type: Error,
+    message: new RegExp('^Could not get or set buffer' +
+      ' size: Error: EINVAL: invalid argument,' +
+      ' uv_' + type + '_buffer_size$')


Nit: I believe a string (instead of a regex) is enough to get exact matching here. You might want to use a template string instead of concatenation, I think we agreed on that preference.

I actually used a template for this but the linter complained which is why I changed it. I'll take another look though.

We usually use templates, except if the line becomes too long, in that case you can concatenate templates and string literals:

'Could not get or set buffer size: Error: EINVAL: ' + `invalid argument, uv_${type}_buffer_size`

Thanks, updated now.

cjihrig · 2017-09-18T13:10:24Z

test/parallel/test-dgram-socket-buffer-size.js

+  const errorObj = {
+    code: 'ERR_SOCKET_BUFFER_SIZE',
+    type: Error,
+    message: `Could not get or set buffer size: Error: EINVAL: invalid argument, uv_${type}_buffer_size` // eslint-disable-line max-len


I think both of the long lines in this file could be handled without disabling the ESLint rule.

danbev · 2017-09-19T07:20:14Z

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

jasnell · 2017-09-19T14:13:02Z

This failed CI across the board.

danbev · 2017-09-20T04:55:48Z

@jasnell Thanks for the heads up. I see the problem and will take a closer look. (I'm at a f2f this week so response times might not be that good)

Currently if the buffer size exceeds the maximum int size the following error will be thrown: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: Unknown system error 22: Unknown system error 22, uv_recv_buffer_size at bufferSize (dgram.js:180:11) It looks like perhaps UV_EINVAL was intended to give the following error message instead: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: EINVAL: invalid argument, uv_recv_buffer_size at bufferSize (dgram.js:180:11) This commit changes EINVAL to use UV_EINVAL.

danbev · 2017-09-20T16:56:34Z

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

BridgeAR · 2017-09-21T15:25:11Z

@danbev seems like the CI is green now. Is this ready to land?

danbev · 2017-09-21T17:09:32Z

Looking into this I found won't work without

@danbev seems like the CI is green now. Is this ready to land?

Yep, it is good to go now. I can land it later this evening or tomorrow.

danbev · 2017-09-21T17:10:12Z

@danbev seems like the CI is green now. Is this ready to land?

Yep, it is good to go now. I can land it later this evening or tomorrow.

BridgeAR · 2017-09-21T22:22:31Z

@danbev top

Currently if the buffer size exceeds the maximum int size the following error will be thrown: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: Unknown system error 22: Unknown system error 22, uv_recv_buffer_size at bufferSize (dgram.js:180:11) It looks like perhaps UV_EINVAL was intended to give the following error message instead: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: EINVAL: invalid argument, uv_recv_buffer_size at bufferSize (dgram.js:180:11) This commit changes EINVAL to use UV_EINVAL. PR-URL: nodejs#15444 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

danbev · 2017-09-22T06:17:57Z

Landed in f4899ac

Currently if the buffer size exceeds the maximum int size the following error will be thrown: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: Unknown system error 22: Unknown system error 22, uv_recv_buffer_size at bufferSize (dgram.js:180:11) It looks like perhaps UV_EINVAL was intended to give the following error message instead: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: EINVAL: invalid argument, uv_recv_buffer_size at bufferSize (dgram.js:180:11) This commit changes EINVAL to use UV_EINVAL. PR-URL: nodejs/node#15444 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Currently if the buffer size exceeds the maximum int size the following error will be thrown: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: Unknown system error 22: Unknown system error 22, uv_recv_buffer_size at bufferSize (dgram.js:180:11) It looks like perhaps UV_EINVAL was intended to give the following error message instead: dgram.js:180 throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e); ^ Error [ERR_SOCKET_BUFFER_SIZE]: Could not get or set buffer size: Error: EINVAL: invalid argument, uv_recv_buffer_size at bufferSize (dgram.js:180:11) This commit changes EINVAL to use UV_EINVAL. PR-URL: #15444 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-10-17T19:12:40Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dgram Issues and PRs related to the dgram subsystem / UDP. labels Sep 17, 2017

tniessen approved these changes Sep 17, 2017

View reviewed changes

lpinca approved these changes Sep 17, 2017

View reviewed changes

addaleax approved these changes Sep 17, 2017

View reviewed changes

cjihrig approved these changes Sep 17, 2017

View reviewed changes

tniessen reviewed Sep 17, 2017

View reviewed changes

cjihrig reviewed Sep 18, 2017

View reviewed changes

jasnell approved these changes Sep 19, 2017

View reviewed changes

danbev added 5 commits September 20, 2017 17:52

squash: add test for recv/send buffer sizes

5178868

squash: use string instread of a regexp

2a76b64

squash: use string template

dfc29b2

squash: remove eslint ignore rule

b87360c

danbev force-pushed the udp_wrap_uv_errors branch from ce843f2 to b87360c Compare September 20, 2017 16:55

danbev closed this Sep 22, 2017

danbev deleted the udp_wrap_uv_errors branch September 22, 2017 06:18

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the backport-requested-v6.x label Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use UV_EINVAL instead of EINVAL in udp_wrap #15444

src: use UV_EINVAL instead of EINVAL in udp_wrap #15444

danbev commented Sep 17, 2017

cjihrig left a comment

danbev commented Sep 17, 2017

tniessen Sep 17, 2017

danbev Sep 18, 2017

tniessen Sep 18, 2017

danbev Sep 19, 2017

cjihrig Sep 18, 2017

danbev commented Sep 19, 2017

jasnell commented Sep 19, 2017

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

BridgeAR commented Sep 21, 2017

danbev commented Sep 21, 2017

danbev commented Sep 21, 2017

BridgeAR commented Sep 21, 2017

danbev commented Sep 22, 2017

MylesBorins commented Oct 17, 2017

src: use UV_EINVAL instead of EINVAL in udp_wrap #15444

src: use UV_EINVAL instead of EINVAL in udp_wrap #15444

Conversation

danbev commented Sep 17, 2017

Checklist

Affected core subsystem(s)

cjihrig left a comment

Choose a reason for hiding this comment

danbev commented Sep 17, 2017

tniessen Sep 17, 2017

Choose a reason for hiding this comment

danbev Sep 18, 2017

Choose a reason for hiding this comment

tniessen Sep 18, 2017

Choose a reason for hiding this comment

danbev Sep 19, 2017

Choose a reason for hiding this comment

cjihrig Sep 18, 2017

Choose a reason for hiding this comment

danbev commented Sep 19, 2017

jasnell commented Sep 19, 2017

danbev commented Sep 20, 2017

danbev commented Sep 20, 2017

BridgeAR commented Sep 21, 2017

danbev commented Sep 21, 2017

danbev commented Sep 21, 2017

BridgeAR commented Sep 21, 2017

danbev commented Sep 22, 2017

MylesBorins commented Oct 17, 2017