Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[x] http: added closed property #28621

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,15 @@ changes:
The `request.aborted` property will be `true` if the request has
been aborted.

### request.closed
<!-- YAML
added: CHANGEME
-->

* {boolean}

The `request.closed` property indicates whether the request has been closed.

### request.connection
<!-- YAML
added: v0.3.0
Expand Down Expand Up @@ -1130,6 +1139,15 @@ response.end();
Attempting to set a header field name or value that contains invalid characters
will result in a [`TypeError`][] being thrown.

### response.closed
<!-- YAML
added: CHANGEME
-->

* {boolean}

The `response.closed` property indicates whether the response has been closed.

### response.connection
<!-- YAML
added: v0.3.0
Expand Down
17 changes: 11 additions & 6 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ function ClientRequest(input, options, cb) {
this._ended = false;
this.res = null;
this.aborted = false;
this.closed = false;
this.timeoutCb = null;
this.upgradeOrConnect = false;
this.parser = null;
Expand Down Expand Up @@ -358,14 +359,12 @@ function socketCloseListener() {
res.aborted = true;
res.emit('aborted');
}
req.emit('close');
emitClose.call(req);
if (res.readable) {
res.on('end', function() {
this.emit('close');
});
res.on('end', emitClose);
res.push(null);
} else {
res.emit('close');
emitClose.call(res);
}
} else {
if (!req.socket._hadError) {
Expand All @@ -375,7 +374,7 @@ function socketCloseListener() {
req.socket._hadError = true;
req.emit('error', connResetException('socket hang up'));
}
req.emit('close');
emitClose.call(req);
}

// Too bad. That output wasn't getting written.
Expand Down Expand Up @@ -417,6 +416,11 @@ function socketErrorListener(err) {
socket.destroy();
}

function emitClose() {
this.closed = true;
this.emit('close');
}

function freeSocketErrorListener(err) {
const socket = this;
debug('SOCKET ERROR on FREE socket:', err.message, err.stack);
Expand Down Expand Up @@ -487,6 +491,7 @@ function socketOnData(d) {
socket.readableFlowing = null;

req.emit(eventName, res, socket, bodyHead);
req.closed = true;
req.emit('close');
} else {
// Requested Upgrade or used CONNECT method, but have no handler.
Expand Down
1 change: 1 addition & 0 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function IncomingMessage(socket) {
this.readable = true;

this.aborted = false;
this.closed = false;

this.upgrade = null;

Expand Down
1 change: 1 addition & 0 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ function OutgoingMessage() {
this._trailer = '';

this.finished = false;
this.closed = false;
this._headerSent = false;
this[kIsCorked] = false;

Expand Down
14 changes: 11 additions & 3 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,9 @@ function onServerResponseClose() {
// Ergo, we need to deal with stale 'close' events and handle the case
// where the ServerResponse object has already been deconstructed.
// Fortunately, that requires only a single if check. :-)
if (this._httpMessage) this._httpMessage.emit('close');
if (this._httpMessage) {
emitClose.call(this._httpMessage);
}
}

ServerResponse.prototype.assignSocket = function assignSocket(socket) {
Expand Down Expand Up @@ -469,7 +471,7 @@ function abortIncoming(incoming) {
var req = incoming.shift();
req.aborted = true;
req.emit('aborted');
req.emit('close');
emitClose.call(req);
}
// Abort socket._httpMessage ?
}
Expand Down Expand Up @@ -594,6 +596,11 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
}
}

function emitClose() {
this.closed = true;
this.emit('close');
}

function resOnFinish(req, res, socket, state, server) {
// Usually the first incoming element should be our request. it may
// be that in the case abortIncoming() was called that the incoming
Expand All @@ -609,7 +616,8 @@ function resOnFinish(req, res, socket, state, server) {
req._dump();

res.detachSocket(socket);
req.emit('close');
res.closed = true;
ronag marked this conversation as resolved.
Show resolved Hide resolved
emitClose.call(req);
process.nextTick(emitCloseNT, res);

if (res._last) {
Expand Down
8 changes: 8 additions & 0 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@ class Http2ServerRequest extends Readable {
this.on('resume', onRequestResume);
}

get closed() {
return this[kState].closed;
}

get aborted() {
return this[kAborted];
}
Expand Down Expand Up @@ -446,6 +450,10 @@ class Http2ServerResponse extends Stream {
stream.on('timeout', onStreamTimeout(kResponse));
}

get closed() {
return this[kState].closed;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests have been added for these.


// User land modules such as finalhandler just check truthiness of this
// but if someone is actually trying to use this for more than that
// then we simply can't support such use cases
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-client-close-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ server.listen(0, common.mustCall(() => {

req.on('close', common.mustCall(() => {
assert.strictEqual(errorEmitted, true);
assert.strictEqual(req.closed, true);
server.close();
}));

Expand Down
17 changes: 16 additions & 1 deletion test/parallel/test-http-connect-req-res.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ server.listen(0, common.mustCall(function() {
path: 'example.com:443'
}, common.mustNotCall());

req.on('close', common.mustCall());
assert.strictEqual(req.closed, false);
req.on('close', common.mustCall(() => {
ronag marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(req.closed, true);
}));

req.on('connect', common.mustCall(function(res, socket, firstBodyChunk) {
assert.strictEqual(req.closed, false);

console.error('Client got CONNECT request');

// Make sure this request got removed from the pool.
Expand All @@ -54,10 +59,20 @@ server.listen(0, common.mustCall(function() {
// Test that the firstBodyChunk was not parsed as HTTP
assert.strictEqual(data, 'Head');

let closeEmitted = false;

req.on('close', common.mustCall(() => {
closeEmitted = true;
}));

socket.on('data', function(buf) {
assert.strictEqual(req.closed, true); // TODO: This should be false
assert.strictEqual(closeEmitted, true); // TODO: This should be false
data += buf.toString();
});
socket.on('end', function() {
assert.strictEqual(req.closed, true); // TODO: This should be false
assert.strictEqual(closeEmitted, true); // TODO: This should be false
assert.strictEqual(data, 'HeadRequestEnd');
server.close();
});
Expand Down
7 changes: 3 additions & 4 deletions test/parallel/test-http-req-res-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,15 @@ const http = require('http');
const assert = require('assert');

const server = http.Server(common.mustCall((req, res) => {
let resClosed = false;

res.end();
res.on('finish', common.mustCall(() => {
assert.strictEqual(resClosed, false);
assert.strictEqual(res.closed, false);
}));
ronag marked this conversation as resolved.
Show resolved Hide resolved
res.on('close', common.mustCall(() => {
resClosed = true;
assert.strictEqual(res.closed, true);
}));
req.on('close', common.mustCall(() => {
assert.strictEqual(req.closed, true);
assert.strictEqual(req._readableState.ended, true);
}));
res.socket.on('close', () => server.close());
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-http-response-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'use strict';
const common = require('../common');
const http = require('http');
const assert = require('assert');

{
const server = http.createServer(
Expand All @@ -40,6 +41,7 @@ const http = require('http');
res.destroy();
}));
res.on('close', common.mustCall(() => {
assert.strictEqual(res.closed, true);
server.close();
}));
})
Expand Down