From 1b2e2944bc069abd1dd7cbf90d3badad4289235d Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 25 Feb 2020 20:18:35 -0500 Subject: [PATCH] dgram: don't hide implicit bind errors When dgram socket implicit binding fails, an attempt is made to clean up the send queue. This was originally implemented using an 'error' handler that performed cleanup and then emitted a fake error, which concealed the original error. This was done to prevent cases where the same error was emitted twice. Now that the errorMonitor event is available, use that to perform the cleanup without impacting the actual error handling. PR-URL: https://github.com/nodejs/node/pull/31958 Refs: https://github.com/nodejs/help/issues/2484 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- lib/dgram.js | 6 +-- .../test-dgram-implicit-bind-failure.js | 47 ++++++------------- 2 files changed, 17 insertions(+), 36 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index 26d1e1d11a0e76..15421de46499d3 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -45,7 +45,6 @@ const { ERR_SOCKET_BAD_BUFFER_SIZE, ERR_SOCKET_BAD_PORT, ERR_SOCKET_BUFFER_SIZE, - ERR_SOCKET_CANNOT_SEND, ERR_SOCKET_DGRAM_IS_CONNECTED, ERR_SOCKET_DGRAM_NOT_CONNECTED, ERR_SOCKET_DGRAM_NOT_RUNNING, @@ -506,7 +505,7 @@ function enqueue(self, toEnqueue) { // event handler that flushes the send queue after binding is done. if (state.queue === undefined) { state.queue = []; - self.once('error', onListenError); + self.once(EventEmitter.errorMonitor, onListenError); self.once('listening', onListenSuccess); } state.queue.push(toEnqueue); @@ -514,7 +513,7 @@ function enqueue(self, toEnqueue) { function onListenSuccess() { - this.removeListener('error', onListenError); + this.removeListener(EventEmitter.errorMonitor, onListenError); clearQueue.call(this); } @@ -522,7 +521,6 @@ function onListenSuccess() { function onListenError(err) { this.removeListener('listening', onListenSuccess); this[kStateSymbol].queue = undefined; - this.emit('error', new ERR_SOCKET_CANNOT_SEND()); } diff --git a/test/sequential/test-dgram-implicit-bind-failure.js b/test/sequential/test-dgram-implicit-bind-failure.js index d77db12618f3bc..89da00d5766fb3 100644 --- a/test/sequential/test-dgram-implicit-bind-failure.js +++ b/test/sequential/test-dgram-implicit-bind-failure.js @@ -2,48 +2,31 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); +const EventEmitter = require('events'); const dgram = require('dgram'); const dns = require('dns'); const { kStateSymbol } = require('internal/dgram'); +const mockError = new Error('fake DNS'); // Monkey patch dns.lookup() so that it always fails. dns.lookup = function(address, family, callback) { - process.nextTick(() => { callback(new Error('fake DNS')); }); + process.nextTick(() => { callback(mockError); }); }; const socket = dgram.createSocket('udp4'); -let dnsFailures = 0; -let sendFailures = 0; -process.on('exit', () => { - assert.strictEqual(dnsFailures, 3); - assert.strictEqual(sendFailures, 3); -}); - -socket.on('error', (err) => { - if (/^Error: fake DNS$/.test(err)) { - // The DNS lookup should fail since it is monkey patched. At that point in - // time, the send queue should be populated with the send() operation. There - // should also be two listeners - this function and the dgram internal one - // time error handler. - dnsFailures++; - assert(Array.isArray(socket[kStateSymbol].queue)); - assert.strictEqual(socket[kStateSymbol].queue.length, 1); - assert.strictEqual(socket.listenerCount('error'), 2); - return; - } - - if (err.code === 'ERR_SOCKET_CANNOT_SEND') { - // On error, the queue should be destroyed and this function should be - // the only listener. - sendFailures++; - assert.strictEqual(socket[kStateSymbol].queue, undefined); - assert.strictEqual(socket.listenerCount('error'), 1); - return; - } - - assert.fail(`Unexpected error: ${err}`); -}); +socket.on(EventEmitter.errorMonitor, common.mustCall((err) => { + // The DNS lookup should fail since it is monkey patched. At that point in + // time, the send queue should be populated with the send() operation. + assert.strictEqual(err, mockError); + assert(Array.isArray(socket[kStateSymbol].queue)); + assert.strictEqual(socket[kStateSymbol].queue.length, 1); +}, 3)); + +socket.on('error', common.mustCall((err) => { + assert.strictEqual(err, mockError); + assert.strictEqual(socket[kStateSymbol].queue, undefined); +}, 3)); // Initiate a few send() operations, which will fail. socket.send('foobar', common.PORT, 'localhost');