Skip to content

Commit

Permalink
http,stream: add writableEnded
Browse files Browse the repository at this point in the history
This is work towards resolving the response.finished confusion and
future deprecation.

Note that implementation-wise, streams have both an ending and ended
state. However, in this case (in order to avoid confusion in user space)
writableEnded is equal to writable.ending. The ending vs ended situation
is internal state required for internal stream logic.

PR-URL: #28934
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
ronag authored and targos committed Aug 19, 2019
1 parent bb19d82 commit f9b61d2
Show file tree
Hide file tree
Showing 11 changed files with 103 additions and 5 deletions.
34 changes: 34 additions & 0 deletions doc/api/http.md
Expand Up @@ -764,6 +764,27 @@ req.once('response', (res) => {
}); });
``` ```


### request.writableEnded
<!-- YAML
added: REPLACEME
-->

* {boolean}

Is `true` after [`request.end()`][] has been called. This property
does not indicate whether the data has been flushed, for this use
[`request.writableFinished`][] instead.

### request.writableFinished
<!-- YAML
added: v12.7.0
-->

* {boolean}

Is `true` if all data has been flushed to the underlying system, immediately
before the [`'finish'`][] event is emitted.

### request.write(chunk[, encoding][, callback]) ### request.write(chunk[, encoding][, callback])
<!-- YAML <!-- YAML
added: v0.1.29 added: v0.1.29
Expand Down Expand Up @@ -1434,6 +1455,17 @@ response.statusMessage = 'Not found';
After response header was sent to the client, this property indicates the After response header was sent to the client, this property indicates the
status message which was sent out. status message which was sent out.


### response.writableEnded
<!-- YAML
added: REPLACEME
-->

* {boolean}

Is `true` after [`response.end()`][] has been called. This property
does not indicate whether the data has been flushed, for this use
[`response.writableFinished`][] instead.

### response.writableFinished ### response.writableFinished
<!-- YAML <!-- YAML
added: v12.7.0 added: v12.7.0
Expand Down Expand Up @@ -2221,11 +2253,13 @@ not abort the request or do anything besides add a `'timeout'` event.
[`request.setTimeout()`]: #http_request_settimeout_timeout_callback [`request.setTimeout()`]: #http_request_settimeout_timeout_callback
[`request.socket.getPeerCertificate()`]: tls.html#tls_tlssocket_getpeercertificate_detailed [`request.socket.getPeerCertificate()`]: tls.html#tls_tlssocket_getpeercertificate_detailed
[`request.socket`]: #http_request_socket [`request.socket`]: #http_request_socket
[`request.writableFinished`]: #http_request_writablefinished
[`request.write(data, encoding)`]: #http_request_write_chunk_encoding_callback [`request.write(data, encoding)`]: #http_request_write_chunk_encoding_callback
[`response.end()`]: #http_response_end_data_encoding_callback [`response.end()`]: #http_response_end_data_encoding_callback
[`response.getHeader()`]: #http_response_getheader_name [`response.getHeader()`]: #http_response_getheader_name
[`response.setHeader()`]: #http_response_setheader_name_value [`response.setHeader()`]: #http_response_setheader_name_value
[`response.socket`]: #http_response_socket [`response.socket`]: #http_response_socket
[`response.writableFinished`]: #http_response_writablefinished
[`response.write()`]: #http_response_write_chunk_encoding_callback [`response.write()`]: #http_response_write_chunk_encoding_callback
[`response.write(data, encoding)`]: #http_response_write_chunk_encoding_callback [`response.write(data, encoding)`]: #http_response_write_chunk_encoding_callback
[`response.writeContinue()`]: #http_response_writecontinue [`response.writeContinue()`]: #http_response_writecontinue
Expand Down
12 changes: 12 additions & 0 deletions doc/api/http2.md
Expand Up @@ -3269,6 +3269,17 @@ added: v8.4.0


The [`Http2Stream`][] object backing the response. The [`Http2Stream`][] object backing the response.


#### response.writableEnded
<!-- YAML
added: REPLACEME
-->

* {boolean}

Is `true` after [`response.end()`][] has been called. This property
does not indicate whether the data has been flushed, for this use
[`writable.writableFinished`][] instead.

#### response.write(chunk[, encoding][, callback]) #### response.write(chunk[, encoding][, callback])
<!-- YAML <!-- YAML
added: v8.4.0 added: v8.4.0
Expand Down Expand Up @@ -3509,3 +3520,4 @@ following additional properties:
[`tls.connect()`]: tls.html#tls_tls_connect_options_callback [`tls.connect()`]: tls.html#tls_tls_connect_options_callback
[`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener [`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
[error code]: #error_codes [error code]: #error_codes
[`writable.writableFinished`]: stream.html#stream_writable_writablefinished
13 changes: 13 additions & 0 deletions doc/api/stream.md
Expand Up @@ -491,6 +491,17 @@ added: v11.4.0


Is `true` if it is safe to call [`writable.write()`][stream-write]. Is `true` if it is safe to call [`writable.write()`][stream-write].


##### writable.writableEnded
<!-- YAML
added: REPLACEME
-->

* {boolean}

Is `true` after [`writable.end()`][] has been called. This property
does not indicate whether the data has been flushed, for this use
[`writable.writableFinished`][] instead.

##### writable.writableFinished ##### writable.writableFinished
<!-- YAML <!-- YAML
added: v12.6.0 added: v12.6.0
Expand Down Expand Up @@ -2704,7 +2715,9 @@ contain multi-byte characters.
[`stream.unpipe()`]: #stream_readable_unpipe_destination [`stream.unpipe()`]: #stream_readable_unpipe_destination
[`stream.wrap()`]: #stream_readable_wrap_stream [`stream.wrap()`]: #stream_readable_wrap_stream
[`writable.cork()`]: #stream_writable_cork [`writable.cork()`]: #stream_writable_cork
[`writable.end()`]: #stream_writable_end_chunk_encoding_callback
[`writable.uncork()`]: #stream_writable_uncork [`writable.uncork()`]: #stream_writable_uncork
[`writable.writableFinished`]: #stream_writable_writablefinished
[`zlib.createDeflate()`]: zlib.html#zlib_zlib_createdeflate_options [`zlib.createDeflate()`]: zlib.html#zlib_zlib_createdeflate_options
[API for Stream Consumers]: #stream_api_for_stream_consumers [API for Stream Consumers]: #stream_api_for_stream_consumers
[API for Stream Implementers]: #stream_api_for_stream_implementers [API for Stream Implementers]: #stream_api_for_stream_implementers
Expand Down
4 changes: 4 additions & 0 deletions lib/_http_outgoing.js
Expand Up @@ -575,6 +575,10 @@ Object.defineProperty(OutgoingMessage.prototype, 'headersSent', {
get: function() { return !!this._header; } get: function() { return !!this._header; }
}); });


Object.defineProperty(OutgoingMessage.prototype, 'writableEnded', {
get: function() { return this.finished; }
});



const crlf_buf = Buffer.from('\r\n'); const crlf_buf = Buffer.from('\r\n');
OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
Expand Down
16 changes: 13 additions & 3 deletions lib/_stream_duplex.js
Expand Up @@ -74,7 +74,7 @@ Object.defineProperty(Duplex.prototype, 'writableHighWaterMark', {
// userland will fail // userland will fail
enumerable: false, enumerable: false,
get() { get() {
return this._writableState.highWaterMark; return this._writableState && this._writableState.highWaterMark;
} }
}); });


Expand All @@ -94,7 +94,7 @@ Object.defineProperty(Duplex.prototype, 'writableLength', {
// userland will fail // userland will fail
enumerable: false, enumerable: false,
get() { get() {
return this._writableState.length; return this._writableState && this._writableState.length;
} }
}); });


Expand All @@ -104,7 +104,17 @@ Object.defineProperty(Duplex.prototype, 'writableFinished', {
// userland will fail // userland will fail
enumerable: false, enumerable: false,
get() { get() {
return this._writableState.finished; return this._writableState ? this._writableState.finished : false;
}
});

Object.defineProperty(Duplex.prototype, 'writableEnded', {
// Making it explicit this property is not enumerable
// because otherwise some prototype manipulation in
// userland will fail
enumerable: false,
get() {
return this._writableState ? this._writableState.ending : false;
} }
}); });


Expand Down
14 changes: 12 additions & 2 deletions lib/_stream_writable.js
Expand Up @@ -352,13 +352,23 @@ function decodeChunk(state, chunk, encoding) {
return chunk; return chunk;
} }


Object.defineProperty(Writable.prototype, 'writableEnded', {
// Making it explicit this property is not enumerable
// because otherwise some prototype manipulation in
// userland will fail
enumerable: false,
get: function() {
return this._writableState ? this._writableState.ending : false;
}
});

Object.defineProperty(Writable.prototype, 'writableHighWaterMark', { Object.defineProperty(Writable.prototype, 'writableHighWaterMark', {
// Making it explicit this property is not enumerable // Making it explicit this property is not enumerable
// because otherwise some prototype manipulation in // because otherwise some prototype manipulation in
// userland will fail // userland will fail
enumerable: false, enumerable: false,
get: function() { get: function() {
return this._writableState.highWaterMark; return this._writableState && this._writableState.highWaterMark;
} }
}); });


Expand Down Expand Up @@ -713,7 +723,7 @@ Object.defineProperty(Writable.prototype, 'writableFinished', {
// userland will fail // userland will fail
enumerable: false, enumerable: false,
get() { get() {
return this._writableState.finished; return this._writableState ? this._writableState.finished : false;
} }
}); });


Expand Down
5 changes: 5 additions & 0 deletions lib/internal/http2/compat.js
Expand Up @@ -454,6 +454,11 @@ class Http2ServerResponse extends Stream {
return this.headersSent; return this.headersSent;
} }


get writableEnded() {
const state = this[kState];
return state.ending;
}

get finished() { get finished() {
const stream = this[kStream]; const stream = this[kStream];
return stream.destroyed || return stream.destroyed ||
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-http-outgoing-finish-writable.js
Expand Up @@ -9,12 +9,14 @@ const http = require('http');
const server = http.createServer(common.mustCall(function(req, res) { const server = http.createServer(common.mustCall(function(req, res) {
assert.strictEqual(res.writable, true); assert.strictEqual(res.writable, true);
assert.strictEqual(res.finished, false); assert.strictEqual(res.finished, false);
assert.strictEqual(res.writableEnded, false);
res.end(); res.end();


// res.writable is set to false after it has finished sending // res.writable is set to false after it has finished sending
// Ref: https://github.com/nodejs/node/issues/15029 // Ref: https://github.com/nodejs/node/issues/15029
assert.strictEqual(res.writable, true); assert.strictEqual(res.writable, true);
assert.strictEqual(res.finished, true); assert.strictEqual(res.finished, true);
assert.strictEqual(res.writableEnded, true);


server.close(); server.close();
})); }));
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-end.js
Expand Up @@ -150,8 +150,10 @@ const {
// for http1 compatibility // for http1 compatibility
const server = createServer(mustCall((request, response) => { const server = createServer(mustCall((request, response) => {
strictEqual(response.finished, true); strictEqual(response.finished, true);
strictEqual(response.writableEnded, false);
response.writeHead(HTTP_STATUS_OK, { foo: 'bar' }); response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
response.end('data', mustCall()); response.end('data', mustCall());
strictEqual(response.writableEnded, true);
})); }));
server.listen(0, mustCall(() => { server.listen(0, mustCall(() => {
const { port } = server.address(); const { port } = server.address();
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-finished.js
Expand Up @@ -25,8 +25,10 @@ server.listen(0, common.mustCall(() => {
})); }));
})); }));
assert.strictEqual(response.finished, false); assert.strictEqual(response.finished, false);
assert.strictEqual(response.writableEnded, false);
response.end(); response.end();
assert.strictEqual(response.finished, true); assert.strictEqual(response.finished, true);
assert.strictEqual(response.writableEnded, true);
})); }));


const url = `http://localhost:${port}`; const url = `http://localhost:${port}`;
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-stream-writable-ended-state.js
Expand Up @@ -9,13 +9,17 @@ const writable = new stream.Writable();


writable._write = (chunk, encoding, cb) => { writable._write = (chunk, encoding, cb) => {
assert.strictEqual(writable._writableState.ended, false); assert.strictEqual(writable._writableState.ended, false);
assert.strictEqual(writable.writableEnded, false);
cb(); cb();
}; };


assert.strictEqual(writable._writableState.ended, false); assert.strictEqual(writable._writableState.ended, false);
assert.strictEqual(writable.writableEnded, false);


writable.end('testing ended state', common.mustCall(() => { writable.end('testing ended state', common.mustCall(() => {
assert.strictEqual(writable._writableState.ended, true); assert.strictEqual(writable._writableState.ended, true);
assert.strictEqual(writable.writableEnded, true);
})); }));


assert.strictEqual(writable._writableState.ended, true); assert.strictEqual(writable._writableState.ended, true);
assert.strictEqual(writable.writableEnded, true);

0 comments on commit f9b61d2

Please sign in to comment.