Skip to content

Commit 2bf0228

Browse files
ronagaddaleax
authored andcommitted
http: move free socket error handling to agent
The http client should not know anything about free sockets. Let the agent handle its pool of sockets. PR-URL: #32003 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent bffc932 commit 2bf0228

File tree

2 files changed

+21
-10
lines changed

2 files changed

+21
-10
lines changed

lib/_http_agent.js

+14
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,13 @@ class ReusedHandle {
5757
}
5858
}
5959

60+
function freeSocketErrorListener(err) {
61+
const socket = this;
62+
debug('SOCKET ERROR on FREE socket:', err.message, err.stack);
63+
socket.destroy();
64+
socket.emit('agentRemove');
65+
}
66+
6067
function Agent(options) {
6168
if (!(this instanceof Agent))
6269
return new Agent(options);
@@ -82,6 +89,11 @@ function Agent(options) {
8289
const name = this.getName(options);
8390
debug('agent.on(free)', name);
8491

92+
// TODO(ronag): socket.destroy(err) might have been called
93+
// before coming here and have an 'error' scheduled. In the
94+
// case of socket.destroy() below this 'error' has no handler
95+
// and could cause unhandled exception.
96+
8597
if (socket.writable &&
8698
this.requests[name] && this.requests[name].length) {
8799
const req = this.requests[name].shift();
@@ -118,6 +130,7 @@ function Agent(options) {
118130
socket.setTimeout(agentTimeout);
119131
}
120132

133+
socket.once('error', freeSocketErrorListener);
121134
freeSockets.push(socket);
122135
} else {
123136
// Implementation doesn't want to keep socket alive
@@ -393,6 +406,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
393406

394407
Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
395408
debug('have free socket');
409+
socket.removeListener('error', freeSocketErrorListener);
396410
req.reusedSocket = true;
397411
socket.ref();
398412
};

lib/_http_client.js

+7-10
Original file line numberDiff line numberDiff line change
@@ -473,13 +473,6 @@ function socketErrorListener(err) {
473473
socket.destroy();
474474
}
475475

476-
function freeSocketErrorListener(err) {
477-
const socket = this;
478-
debug('SOCKET ERROR on FREE socket:', err.message, err.stack);
479-
socket.destroy();
480-
socket.emit('agentRemove');
481-
}
482-
483476
function socketOnEnd() {
484477
const socket = this;
485478
const req = this._httpMessage;
@@ -658,8 +651,11 @@ function responseKeepAlive(req) {
658651
socket.removeListener('error', socketErrorListener);
659652
socket.removeListener('data', socketOnData);
660653
socket.removeListener('end', socketOnEnd);
661-
socket.once('error', freeSocketErrorListener);
662-
// There are cases where _handle === null. Avoid those. Passing null to
654+
655+
// TODO(ronag): Between here and emitFreeNT the socket
656+
// has no 'error' handler.
657+
658+
// There are cases where _handle === null. Avoid those. Passing undefined to
663659
// nextTick() will call getDefaultTriggerAsyncId() to retrieve the id.
664660
const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined;
665661
// Mark this socket as available, AFTER user-added end
@@ -741,7 +737,6 @@ function tickOnSocket(req, socket) {
741737
}
742738

743739
parser.onIncoming = parserOnIncomingClient;
744-
socket.removeListener('error', freeSocketErrorListener);
745740
socket.on('error', socketErrorListener);
746741
socket.on('data', socketOnData);
747742
socket.on('end', socketOnEnd);
@@ -781,6 +776,8 @@ function listenSocketTimeout(req) {
781776
}
782777

783778
ClientRequest.prototype.onSocket = function onSocket(socket) {
779+
// TODO(ronag): Between here and onSocketNT the socket
780+
// has no 'error' handler.
784781
process.nextTick(onSocketNT, this, socket);
785782
};
786783

0 commit comments

Comments
 (0)