Closing UDP socket with abort signal can lead to uncaught exception

[pasieronen]

Version

v18.14.1

Platform

Darwin ***.local 22.3.0 Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:37 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T6000 arm64

Subsystem

dgram

What steps will reproduce the bug?

const dgram = require('dgram')
try {
  const controller = new AbortController()
  const socket = dgram.createSocket({type: 'udp4', signal: controller.signal})
  socket.on('error', e => console.log('socket error', e))
  socket.close()
  console.log('socket closed')
  controller.abort()
  console.log('aborted')
} catch (e) {
  console.log('caught error', e)
}

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Socket is closed, no errors.

What do you see instead?

% node test.js
socket closed
aborted
node:internal/event_target:1010
  process.nextTick(() => { throw err; });
                           ^

Error [ERR_SOCKET_DGRAM_NOT_RUNNING]: Not running
    at new NodeError (node:internal/errors:399:5)
    at healthCheck (node:dgram:911:11)
    at Socket.close (node:dgram:743:3)
    at EventTarget.onAborted (node:dgram:142:12)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:735:20)
    at EventTarget.dispatchEvent (node:internal/event_target:677:26)
    at abortSignal (node:internal/abort_controller:308:10)
    at AbortController.abort (node:internal/abort_controller:338:5)
    at Object.<anonymous> (/Users/peronen/Downloads/node-signal/test.js:8:14)
    at Module._compile (node:internal/modules/cjs/loader:1254:14) {
  code: 'ERR_SOCKET_DGRAM_NOT_RUNNING'
}

Node.js v18.14.1

Additional information

The code in dgram.js (https://github.com/nodejs/node/blob/main/lib/dgram.js#L148) tries to make
aborting an already-closed socket a no-op. I think what's happening is that socket.close() closes the socket but emitting the close event happens asynchronously - so it's possible onAborted gets run before the event, and tries to close the socket again (which fails).

Maybe just add try-catch to onAborted, ignoring any exceptions thrown by socket.close()?

[bnoordhuis]

Should be fixable with this diff:

diff --git a/lib/dgram.js b/lib/dgram.js
index 4d87299046d..854fc238066 100644
--- a/lib/dgram.js
+++ b/lib/dgram.js
@@ -139,7 +139,7 @@ function Socket(type, listener) {
     const { signal } = options;
     validateAbortSignal(signal, 'options.signal');
     const onAborted = () => {
-      this.close();
+      if (this[kStateSymbol].handle) this.close();
     };
     if (signal.aborted) {
       onAborted();

Pull request welcome.

[vramana]

I'll work on this issue.