dgram: copy the array when using the list variant in send()#6804

mcollina · 2016-05-17T10:55:16Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

dgram

Description of change

This commit fix a possible crash situation in dgram send().
A crash is possible if an array is passed, and then altered after the
send call, as the call to libuv is wrapped in process.nextTick().

This PR slightly slow things down (1-2%) on Mac OS X, let me know if we should do a full performance review.

Fixes: #6616

cc @bnoordhuis @brown-dragon

jasnell · 2016-05-17T14:58:19Z

LGTM if CI is green.

mcollina · 2016-05-17T15:21:17Z

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

mcollina · 2016-05-17T15:21:56Z

This should be backported to v5.

mcollina · 2016-05-19T07:09:03Z

There are some failures on freebsd and smartos, I think they are transient, rerunning: https://ci.nodejs.org/job/node-test-pull-request/2697/

bnoordhuis · 2016-05-19T08:32:13Z

LGTM but bonus points if you also add a fix + regression test for the empty array case. It's okay to do it in a separate commit because it's arguably a separate issue.

Aside: I'm slightly surprised the linter accepts the assign-in-if syntax. But if the linter is okay with it, then I am, too.

mcollina · 2016-05-19T09:33:57Z

@bnoordhuis done, I'm adding an empty buffer if the array is empty, or do you think that throwing will be better?

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

bnoordhuis · 2016-05-19T09:46:50Z

lib/dgram.js

It's more of a personal preference but can you use buffer.push(...)? That makes it clear we're appending, not replacing, without having to look at the surrounding code.

bnoordhuis · 2016-05-19T09:51:46Z

A small nit about the first commit: the first line of the commit log should be <= 50 characters.

While you're working on send(), can I suggest introducing a local variable called e.g. list? Replacing the buffer argument with the processed result is less clear, IMO.

mcollina · 2016-05-19T13:50:02Z

@bnoordhuis update with the nit you asked!

Let me know if you are ok with this, and I'll land it.

santigimeno · 2016-05-19T13:52:19Z

test/parallel/test-dgram-send-empty-array.js

Can we get rid of the timer?

Yes. It allows fast-failures, otherwise you rely on the 1 minute timeout that each tests has.

Yes, but these kind of timers have been source of flaky tests. I don't think relying in the test runner timeout is a bad idea.

The problem are not the timers, UDP messages can get lost: that's why the tests are flaky. If it has not arrived after 200ms (adjusted for platforms), it will never arrive. We might discuss on increasing those timeouts to reduce the likelyhood of missing a "right" message.
There will always be flaky tests on UDP if we want high coverage.

I understand that: it's UDP.
But I think these kind timers (the test is taking longer than expected so we throw) should only exist when testing timing related features (timers, connection timers, etc.) otherwise the test runner should catch them. I think @Trott made some good points here: nodejs/testing#27 (comment).
Anyway, I'm fine either way.

All tests of UDP are done in the same way. If we want to change those, we can (and probably we should). It's definite a different PR as this is becoming more and more full of stuff.

It makes sense to me.

mcollina · 2016-05-19T14:15:13Z

@santigimeno I've updated the tests.

BTW, should we kick off @nodejs/dgram?

Let me know if anybody would like to change anything else :)

These looks like 4 independent commits, what metadata should I apply to those in the message editing phase?

mcollina · 2016-05-19T14:16:05Z

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

bnoordhuis · 2016-05-19T17:12:01Z

lib/dgram.js

list is a better name for the argument and, arguably, fixBufferList for the function.

bnoordhuis · 2016-05-19T17:16:37Z

There are failures on the OS X and ARM buildbots. The FreeBSD test failure appears to be unrelated.

mcollina · 2016-05-23T07:13:33Z

@bnoordhuis addressed your nits.

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

bnoordhuis · 2016-05-23T07:56:34Z

test/parallel/test-dgram-send-callback-buffer-length.js

common.platformTimeout(200)

bnoordhuis · 2016-05-23T07:58:53Z

LGTM with comment. Was your plan to squash the last two commits into the first two?

mcollina · 2016-05-23T08:08:17Z

@bnoordhuis I still see 4 commits, or maybe I did some mistake on git and github is not picking that up.

mcollina · 2016-05-23T08:13:35Z

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

I did a couple of fix to avoid some flakyness.

mcollina · 2016-05-23T08:22:36Z

new CI: https://ci.nodejs.org/job/node-test-pull-request/2737/
(test was failing in the previous one)

mcollina · 2016-05-23T09:08:59Z

new new CI: https://ci.nodejs.org/job/node-test-pull-request/2739/
(sigh, lots of unrelated failures on SmartOS & others :/)

mcollina · 2016-05-23T10:09:52Z

Again with some changes: https://ci.nodejs.org/job/node-test-pull-request/2740/

mcollina · 2016-05-24T17:02:10Z

@nodejs/build can you please have a look at the 'parallel/test-dgram-send-empty-array ' test in this PR on OS X? I think it should pass, but it is currently failing and I cannot understand why. I've run it thousands of times here and I got zero failures.

This is the last run: https://ci.nodejs.org/job/node-test-commit-osx/3487/nodes=osx1010/tapTestReport/

cjihrig · 2016-05-24T17:09:26Z

Any chance that this is nodejs/node-v0.x-archive#8023 resurfacing? You could try removing the skip in https://github.com/nodejs/node/blob/master/test/parallel/test-dgram-empty-packet.js and running the CI on OS X.

mcollina · 2016-05-24T17:24:36Z

@cjihrig it should be it. Maybe the CI box is not running El Capitan?

mcollina · 2016-05-24T17:59:48Z

Thanks @cjihrig for the support, that was it.

New CI with the fix: https://ci.nodejs.org/job/node-test-pull-request/2770/

mcollina · 2016-05-24T18:37:55Z

ci passing, @jasnell @bnoordhuis please give a last pass before I merge.

bnoordhuis · 2016-05-24T20:31:28Z

LGTM but I only gave it a real quick look-over this time around.

This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). Fixes: nodejs#6616

Sending an empty array was crashing in libuv, this commit allocates an empty buffer to allow sending an empty array.

nits about variable names and push()

Fixes situations were some events were not asserted to be emitted. Removed some flakyness from tests.

mcollina · 2016-05-25T08:00:44Z

CI after rebase https://ci.nodejs.org/job/node-test-pull-request/2777/ (didn't apply cleanly)

jasnell · 2016-05-25T16:43:59Z

LGTM

jasnell · 2016-05-25T18:16:28Z

@mcollina ... saw your question in IRC re: squashing the commits or not.... I would go ahead and squash before landing

mcollina · 2016-05-26T09:29:32Z

Landed as c14e98b.

This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). It also avoid sending an empty array to libuv by allocating an empty buffer. It also does some cleanup inside send() to increase readability. It removes test flakyness by use common.mustCall and common.platformTimeout. Fixes situations were some events were not asserted to be emitted. Fixes: #6616 PR-URL: #6804 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). It also avoid sending an empty array to libuv by allocating an empty buffer. It also does some cleanup inside send() to increase readability. It removes test flakyness by use common.mustCall and common.platformTimeout. Fixes situations were some events were not asserted to be emitted. Fixes: nodejs#6616 PR-URL: nodejs#6804 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). It also avoid sending an empty array to libuv by allocating an empty buffer. It also does some cleanup inside send() to increase readability. It removes test flakyness by use common.mustCall and common.platformTimeout. Fixes situations were some events were not asserted to be emitted. Fixes: #6616 PR-URL: #6804 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2016-07-11T21:43:05Z

@mcollina lts?

mcollina · 2016-07-12T06:30:05Z

@thealphanerd this depends on #4374, which hasn't been ported yet. I'm 👍 on both.

MylesBorins · 2016-07-12T16:49:04Z

I'm going to assume that #4374 will not land in LTS as it does not increase stability. As such I will label this as don't land for now.
Please feel free to open a backport PR or issue on the LTS repo so the WG can discuss it if you think we should include it in v4.5.0

mcollina · 2016-07-12T17:31:26Z

I'm neutral, the only reason to have it on the v4 like is to encourage adoption of this API. It's not that useful with support only on v5 and v6. But, I agree with you that it does not increase stability.

nodejs-github-bot added the dgram Issues and PRs related to the dgram subsystem / UDP. label May 17, 2016

bnoordhuis reviewed May 19, 2016
View reviewed changes

mcollina force-pushed the dgram-multi-buffer-copy branch from c9e6e80 to 09604f8Compare May 19, 2016 13:33

santigimeno reviewed May 19, 2016
View reviewed changes

bnoordhuis reviewed May 19, 2016
View reviewed changes

lib/dgram.js Outdated
Copy link
Member
bnoordhuisMay 19, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list is a better name for the argument and, arguably, fixBufferList for the function.

mcollina force-pushed the dgram-multi-buffer-copy branch from 27e05fb to 05ad943Compare May 23, 2016 07:12

bnoordhuis reviewed May 23, 2016
View reviewed changes

test/parallel/test-dgram-send-callback-buffer-length.js Outdated
Copy link
Member
bnoordhuisMay 23, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.platformTimeout(200)

mcollina force-pushed the dgram-multi-buffer-copy branch from 05ad943 to 4bbdb54Compare May 23, 2016 08:12

mcollina force-pushed the dgram-multi-buffer-copy branch from 4bbdb54 to 8a3d435Compare May 23, 2016 08:22

mcollina force-pushed the dgram-multi-buffer-copy branch from 8a3d435 to bcaa1c9Compare May 23, 2016 10:09

mcollina force-pushed the dgram-multi-buffer-copy branch from 7e1a6c9 to 1b8c3e2Compare May 24, 2016 17:24

mcollina force-pushed the dgram-multi-buffer-copy branch from 1b8c3e2 to 7b65d94Compare May 24, 2016 17:58

mcollina added 4 commits May 25, 2016 09:35

dgram: copy the list in send
353c6e2
This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). Fixes: nodejs#6616

dgram: support sending an empty array
1059d07
Sending an empty array was crashing in libuv, this commit allocates an empty buffer to allow sending an empty array.

dgram: renamed buffer -> list in send
0f5f917
nits about variable names and push()

dgram: use common.mustCall and common.platformTimeout in send tests
e1a4eee
Fixes situations were some events were not asserted to be emitted. Removed some flakyness from tests.

mcollina force-pushed the dgram-multi-buffer-copy branch from 7b65d94 to e1a4eeeCompare May 25, 2016 07:45

mcollina closed this May 26, 2016

MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed

MylesBorins added the lts-watch-v4.x label Jul 12, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jul 12, 2016

Uh oh!

dgram: copy the array when using the list variant in send()#6804

dgram: copy the array when using the list variant in send() #6804

Uh oh!

Conversation

mcollina commented May 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented May 17, 2016

Uh oh!

mcollina commented May 17, 2016

Uh oh!

mcollina commented May 17, 2016

Uh oh!

mcollina commented May 19, 2016

Uh oh!

bnoordhuis commented May 19, 2016

Uh oh!

mcollina commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 19, 2016

Uh oh!

mcollina commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 19, 2016

Uh oh!

mcollina commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 19, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 23, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

mcollina commented May 23, 2016

Uh oh!

mcollina commented May 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented May 24, 2016

Uh oh!

mcollina commented May 24, 2016

Uh oh!

mcollina commented May 24, 2016

Uh oh!

mcollina commented May 24, 2016

Uh oh!

bnoordhuis commented May 24, 2016

Uh oh!

mcollina commented May 17, 2016•
edited
Loading

mcollina commented May 24, 2016•
edited
Loading