Skip to content

Commit

Permalink
Assign event listener to socket close event on open before attempting…
Browse files Browse the repository at this point in the history
… post-open logic (#1186)

* Assign event listener to socket close event on open before attempting to execute post-connection logic, prottects possible unhandled rejection in disconnect
* Remove try/catch for linting failure
* Up timeout for CI failure
* Feedback emit error for disconnection failure on connect failure
* Feedback ignore error on disconnect, propagate root cause
* Feedback add extra test check for expected error on connect
* Feedback remove setTimeout for await reconnect
  • Loading branch information
nickewansmith authored and 0xCLARITY committed Jan 28, 2020
1 parent 97ca0f0 commit c17827e
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 12 deletions.
23 changes: 11 additions & 12 deletions src/common/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -489,18 +489,6 @@ export class Connection extends EventEmitter {
this._ws.on('error', error =>
this.emit('error', 'websocket', error.message, error)
)
// Finalize the connection and resolve all awaiting connect() requests
try {
this._retryConnectionBackoff.reset()
await this._subscribeToLedger()
this._startHeartbeatInterval()
this._connectionManager.resolveAllAwaiting()
this.emit('connected')
} catch (error) {
this._connectionManager.rejectAllAwaiting(error)
this.disconnect()
return
}
// Handle a closed connection: reconnect if it was unexpected
this._ws.once('close', code => {
this._clearHeartbeatInterval()
Expand All @@ -524,6 +512,17 @@ export class Connection extends EventEmitter {
}, retryTimeout)
}
})
// Finalize the connection and resolve all awaiting connect() requests
try {
this._retryConnectionBackoff.reset()
await this._subscribeToLedger()
this._startHeartbeatInterval()
this._connectionManager.resolveAllAwaiting()
this.emit('connected')
} catch (error) {
this._connectionManager.rejectAllAwaiting(error)
await this.disconnect().catch(() => {}) // Ignore this error, propagate the root cause.
}
})
return this._connectionManager.awaitConnection()
}
Expand Down
18 changes: 18 additions & 0 deletions test/connection-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,24 @@ describe('Connection', function() {
}
)

it('should clean up websocket connection if error after websocket is opened', async function() {
await this.api.disconnect();
// fail on connection
this.api.connection._subscribeToLedger = async () => {
throw new Error('error on _subscribeToLedger')
}
try {
await this.api.connect();
throw new Error('expected connect() to reject, but it resolved')
} catch (err) {
assert(err.message === 'error on _subscribeToLedger');
// _ws.close event listener should have cleaned up the socket when disconnect _ws.close is run on connection error
// do not fail on connection anymore
this.api.connection._subscribeToLedger = async () => {}
await this.api.connection.reconnect();
}
})

it('should try to reconnect on empty subscribe response on reconnect', function(done) {
this.timeout(23000)
this.api.on('error', error => {
Expand Down

0 comments on commit c17827e

Please sign in to comment.