dgram: generalized send queue to handle close #7066

mcollina · 2016-05-30T16:34:42Z

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

If the udp socket is not ready and we are accumulating
messages to send, it needs to delay closing the socket when
all messages are flushed.

Fixes: #7061

If the udp socket is not ready and we are accumulating messages to send, it needs to delay closing the socket when all messages are flushed. Fixes: nodejs#7061

mcollina · 2016-05-30T16:35:17Z

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

mcollina · 2016-05-30T16:35:39Z

cc @bnoordhuis @jasnell

ronkorving · 2016-05-31T08:19:42Z

lib/dgram.js

-      this._sendQueue = undefined;
-    });
+  if (!self._queue) {
+    self._queue = [];


Nothing new in this PR, but this looks like a deopt.

why?
Plus, this is used during startup only, so I see little harm in deopting there.

The Socket class doesn't have a _queue property in its constructor. I didn't mean a deopt on this function, but on the hidden class for Socket.

Yes, I wouldn't worry here, as it will get optimized on the long run, and
it does not add a huge delay during "boot" of a Socket.
Il giorno mar 31 mag 2016 alle 12:00 Ron Korving [email protected]
ha scritto:

In lib/dgram.js
#7066 (comment):

@@ -283,20 +283,25 @@ function fixBufferList(list) {
function enqueue(self, toEnqueue) {
// If the send queue hasn't been initialized yet, do it, and install an
// event handler that flushes the send queue after binding is done.

if (!self._sendQueue) {

self._sendQueue = [];

self.once('listening', function() {

// Flush the send queue.

for (var i = 0; i < this._sendQueue.length; i++)

this.send.apply(self, this._sendQueue[i]);

this._sendQueue = undefined;

});

if (!self._queue) {

self._queue = [];

The Socket class doesn't have a _queue property in its constructor. I
didn't mean a deopt on this function, but on the hidden class for Socket.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/7066/files/0e094acf5bed6a847c655fc69c33067e29227f60#r65153703,
or mute the thread
https://github.com/notifications/unsubscribe/AADL4wacLUEUMxSKPGvIT5WbfZubJhNZks5qHAaigaJpZM4Ip7BT
.

mcollina · 2016-05-31T09:33:58Z

I'm flagging this semver-minor to be conservative, especially if we decide to backport it. It changes how the "delayed open" works in dgram, so it might cause breakages we do not expect.

mcollina · 2016-07-13T14:30:26Z

@nodejs/collaborators can I get a couple of LGTMs and get that merged?

i5ting · 2016-07-13T14:31:51Z

LGTM

addaleax · 2016-07-13T14:34:35Z

test/parallel/test-dgram-close-in-listening.js

@@ -0,0 +1,18 @@
+'use strict';
+// Ensure that if a dgram socket is closed before the sendQueue is drained
+// wil not crash


typo: will

addaleax · 2016-07-13T14:41:47Z

LGTM

mcollina · 2016-07-13T15:52:06Z

Landed as a2a711a

If the udp socket is not ready and we are accumulating messages to send, it needs to delay closing the socket when all messages are flushed. Fixes: #7061 PR-URL: #7066 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2016-10-10T16:32:52Z

@mcollina do you still believe this should be backported? Can you quickly explain why it is necessary if so? All semver-minor commits need to be agreed upon by the lts working group, so whatever info you can give will be helpful in making the decision

mcollina · 2016-10-10T20:15:20Z

It can be backported. I think it should be backported, because it fixes a bad crash. It's semver minor because it defines some behavior that was unspecified before. I think can leave without, but if you are bumping semver-minor, you might consider letting this in.

MylesBorins · 2016-11-18T20:26:48Z

@mcollina are you able to backport this? It is a bit time sensitive as we want to get the commit into the v4.7.0 release which has an r.c. going out Tuesday. Sorry for the rush.

If the udp socket is not ready and we are accumulating messages to send, it needs to delay closing the socket when all messages are flushed. Fixes: nodejs#7061 PR-URL: nodejs#7066 Reviewed-By: Anna Henningsen <[email protected]>

If the udp socket is not ready and we are accumulating messages to send, it needs to delay closing the socket when all messages are flushed. Fixes: #7061 PR-URL: #7066 Reviewed-By: Anna Henningsen <[email protected]>

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) nodejs/node#7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) nodejs/node#3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) nodejs/node#7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) nodejs/node#4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) nodejs/node#6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) nodejs/node#8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9736 Signed-off-by: Ilkka Myller <[email protected]>

dgram: generalized send queue to handle close

0e094ac

If the udp socket is not ready and we are accumulating messages to send, it needs to delay closing the socket when all messages are flushed. Fixes: nodejs#7061

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

ronkorving reviewed May 31, 2016
View reviewed changes

mcollina added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v4.x labels May 31, 2016

addaleax reviewed Jul 13, 2016
View reviewed changes

mcollina closed this Jul 13, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the lts-agenda label Oct 10, 2016

MylesBorins added this to the 4.7.0 milestone Nov 18, 2016

mcollina deleted the dgram-queue branch November 19, 2016 07:48

mcollina mentioned this pull request Nov 19, 2016

dgram: generalized send queue to handle close #9695

Closed

3 tasks

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

MylesBorins removed this from the 4.7.0 milestone Nov 22, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v4.7.0 proposal #9736

Merged

MichaelSun90 mentioned this pull request Mar 3, 2020

refactor: random test failures refactor tediousjs/tedious#988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dgram: generalized send queue to handle close #7066

dgram: generalized send queue to handle close #7066

mcollina commented May 30, 2016

mcollina commented May 30, 2016

mcollina commented May 30, 2016

ronkorving May 31, 2016

mcollina May 31, 2016

ronkorving May 31, 2016

mcollina May 31, 2016

mcollina commented May 31, 2016

mcollina commented Jul 13, 2016

i5ting commented Jul 13, 2016

addaleax Jul 13, 2016

addaleax commented Jul 13, 2016

mcollina commented Jul 13, 2016

MylesBorins commented Oct 10, 2016

mcollina commented Oct 10, 2016

MylesBorins commented Nov 18, 2016

dgram: generalized send queue to handle close #7066

dgram: generalized send queue to handle close #7066

Conversation

mcollina commented May 30, 2016

Checklist

Affected core subsystem(s)

Description of change

mcollina commented May 30, 2016

mcollina commented May 30, 2016

ronkorving May 31, 2016

Choose a reason for hiding this comment

mcollina May 31, 2016

Choose a reason for hiding this comment

ronkorving May 31, 2016

Choose a reason for hiding this comment

mcollina May 31, 2016

Choose a reason for hiding this comment

mcollina commented May 31, 2016

mcollina commented Jul 13, 2016

i5ting commented Jul 13, 2016

addaleax Jul 13, 2016

Choose a reason for hiding this comment

addaleax commented Jul 13, 2016

mcollina commented Jul 13, 2016

MylesBorins commented Oct 10, 2016

mcollina commented Oct 10, 2016

MylesBorins commented Nov 18, 2016