Skip to content

Commit

Permalink
fix: make tcp closing error throwing consistent (#2729)
Browse files Browse the repository at this point in the history
In the first close attempt we wrap the `closePromise` with a try/catch
and call `.abort` if it throws, and we don't propagate the error.

In the second invocation we return the `closePromise` which can reject
making the code non-deterministic.
  • Loading branch information
achingbrain authored Sep 27, 2024
1 parent 82bd42b commit 9308bc1
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 21 deletions.
2 changes: 2 additions & 0 deletions packages/transport-tcp/src/listener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,10 @@ export class TCPListener extends TypedEventEmitter<ListenerEvents> implements Li

private onSocket (socket: net.Socket): void {
if (this.status.code !== TCPListenerStatusCode.ACTIVE) {
socket.destroy()
throw new NotStartedError('Server is not listening yet')
}

// Avoid uncaught errors caused by unstable connections
socket.on('error', err => {
this.log('socket error', err)
Expand Down
47 changes: 26 additions & 21 deletions packages/transport-tcp/src/socket-to-conn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,47 +127,48 @@ export const toMultiaddrConnection = (socket: Socket, options: ToConnectionOptio
timeline: { open: Date.now() },

async close (options: AbortOptions = {}) {
if (socket.destroyed) {
log('The %s socket is destroyed', lOptsStr)
if (socket.closed) {
log('The %s socket is already closed', lOptsStr)
return
}

if (closePromise != null) {
log('The %s socket is closed or closing', lOptsStr)
return closePromise
}

if (options.signal == null) {
const signal = AbortSignal.timeout(closeTimeout)

options = {
...options,
signal
}
if (socket.destroyed) {
log('The %s socket is already destroyed', lOptsStr)
return
}

const abortSignalListener = (): void => {
socket.destroy(new AbortError('Destroying socket after timeout'))
}

options.signal?.addEventListener('abort', abortSignalListener)

try {
if (closePromise != null) {
log('The %s socket is already closing', lOptsStr)
await closePromise
return
}

if (options.signal == null) {
const signal = AbortSignal.timeout(closeTimeout)

options = {
...options,
signal
}
}

options.signal?.addEventListener('abort', abortSignalListener)

log('%s closing socket', lOptsStr)
closePromise = new Promise<void>((resolve, reject) => {
socket.once('close', () => {
// socket completely closed
log('%s socket closed', lOptsStr)

resolve()
})
socket.once('error', (err: Error) => {
log('%s socket error', lOptsStr, err)

// error closing socket
if (maConn.timeline.close == null) {
maConn.timeline.close = Date.now()
}
if (!socket.destroyed) {
reject(err)
}
Expand Down Expand Up @@ -210,6 +211,10 @@ export const toMultiaddrConnection = (socket: Socket, options: ToConnectionOptio
socket.destroy(err)
}

// closing a socket is always asynchronous (must wait for "close" event)
// but the tests expect this to be a synchronous operation so we have to
// set the close time here. the tests should be refactored to reflect
// reality.
if (maConn.timeline.close == null) {
maConn.timeline.close = Date.now()
}
Expand Down

0 comments on commit 9308bc1

Please sign in to comment.