Skip to content

Commit 74b0e8c

Browse files
ronagtargos
authored andcommitted
http: ensure client request emits close
If socket creation failed then an error would be emitted on the client request object, but not 'close' nor would destroyed be set to true. PR-URL: #33178 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent 60ebbc4 commit 74b0e8c

File tree

3 files changed

+43
-25
lines changed

3 files changed

+43
-25
lines changed

lib/_http_agent.js

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,12 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
242242
} else if (sockLen < this.maxSockets) {
243243
debug('call onSocket', sockLen, freeLen);
244244
// If we are under maxSockets create a new one.
245-
this.createSocket(req, options, handleSocketCreation(this, req, true));
245+
this.createSocket(req, options, (err, socket) => {
246+
if (err)
247+
req.onSocket(socket, err);
248+
else
249+
setRequestSocket(this, req, socket);
250+
});
246251
} else {
247252
debug('wait for socket');
248253
// We are over limit so we'll add it to the queue.
@@ -388,8 +393,12 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
388393
debug('removeSocket, have a request, make a socket');
389394
const req = this.requests[name][0];
390395
// If we have pending requests and a socket gets closed make a new one
391-
const socketCreationHandler = handleSocketCreation(this, req, false);
392-
this.createSocket(req, options, socketCreationHandler);
396+
this.createSocket(req, options, (err, socket) => {
397+
if (err)
398+
req.onSocket(socket, err);
399+
else
400+
socket.emit('free');
401+
});
393402
}
394403
};
395404

@@ -422,19 +431,6 @@ Agent.prototype.destroy = function destroy() {
422431
}
423432
};
424433

425-
function handleSocketCreation(agent, request, informRequest) {
426-
return function handleSocketCreation_Inner(err, socket) {
427-
if (err) {
428-
process.nextTick(emitErrorNT, request, err);
429-
return;
430-
}
431-
if (informRequest)
432-
setRequestSocket(agent, request, socket);
433-
else
434-
socket.emit('free');
435-
};
436-
}
437-
438434
function setRequestSocket(agent, req, socket) {
439435
req.onSocket(socket);
440436
const agentTimeout = agent.options.timeout || 0;
@@ -444,10 +440,6 @@ function setRequestSocket(agent, req, socket) {
444440
socket.setTimeout(req.timeout);
445441
}
446442

447-
function emitErrorNT(emitter, err) {
448-
emitter.emit('error', err);
449-
}
450-
451443
module.exports = {
452444
Agent,
453445
globalAgent: new Agent()

lib/_http_client.js

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -371,10 +371,12 @@ function _destroy(req, socket, err) {
371371
// TODO (ronag): Check if socket was used at all (e.g. headersSent) and
372372
// re-use it in that case. `req.socket` just checks whether the socket was
373373
// assigned to the request and *might* have been used.
374-
if (!req.agent || req.socket) {
374+
if (socket && (!req.agent || req.socket)) {
375375
socket.destroy(err);
376376
} else {
377-
socket.emit('free');
377+
if (socket) {
378+
socket.emit('free');
379+
}
378380
if (!req.aborted && !err) {
379381
err = connResetException('socket hang up');
380382
}
@@ -776,15 +778,18 @@ function listenSocketTimeout(req) {
776778
}
777779
}
778780

779-
ClientRequest.prototype.onSocket = function onSocket(socket) {
781+
ClientRequest.prototype.onSocket = function onSocket(socket, err) {
780782
// TODO(ronag): Between here and onSocketNT the socket
781783
// has no 'error' handler.
782-
process.nextTick(onSocketNT, this, socket);
784+
process.nextTick(onSocketNT, this, socket, err);
783785
};
784786

785-
function onSocketNT(req, socket) {
787+
function onSocketNT(req, socket, err) {
786788
if (req.destroyed) {
787789
_destroy(req, socket, req[kError]);
790+
} else if (err) {
791+
req.destroyed = true;
792+
_destroy(req, null, err);
788793
} else {
789794
tickOnSocket(req, socket);
790795
}
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const http = require('http');
5+
6+
const agent = new http.Agent();
7+
const _err = new Error('kaboom');
8+
agent.createSocket = function(req, options, cb) {
9+
cb(_err);
10+
};
11+
12+
const req = http
13+
.request({
14+
agent
15+
})
16+
.on('error', common.mustCall((err) => {
17+
assert.strictEqual(err, _err);
18+
}))
19+
.on('close', common.mustCall(() => {
20+
assert.strictEqual(req.destroyed, true);
21+
}));

0 commit comments

Comments
 (0)