From 6f613d8abb05619e35c828b665a76215861bbf04 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Fri, 2 Aug 2019 08:09:06 +0200
Subject: [PATCH] http,stream: add writableEnded

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: https://github.com/nodejs/node/pull/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>
---
 doc/api/http.md                               | 34 +++++++++++++++++++
 doc/api/http2.md                              | 12 +++++++
 doc/api/stream.md                             | 13 +++++++
 lib/_http_outgoing.js                         |  4 +++
 lib/_stream_duplex.js                         | 16 +++++++--
 lib/_stream_writable.js                       | 14 ++++++--
 lib/internal/http2/compat.js                  |  5 +++
 .../test-http-outgoing-finish-writable.js     |  2 ++
 .../test-http2-compat-serverresponse-end.js   |  2 ++
 ...st-http2-compat-serverresponse-finished.js |  2 ++
 .../test-stream-writable-ended-state.js       |  4 +++
 11 files changed, 103 insertions(+), 5 deletions(-)

diff --git a/doc/api/http.md b/doc/api/http.md
index 4923b6a61d840f..d5851b61dbba3c 100644
--- a/doc/api/http.md
+++ b/doc/api/http.md
@@ -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])
 <!-- YAML
 added: v0.1.29
@@ -1436,6 +1457,17 @@ response.statusMessage = 'Not found';
 After response header was sent to the client, this property indicates the
 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
 <!-- YAML
 added: v12.7.0
@@ -2222,11 +2254,13 @@ not abort the request or do anything besides add a `'timeout'` event.
 [`request.setTimeout()`]: #http_request_settimeout_timeout_callback
 [`request.socket.getPeerCertificate()`]: tls.html#tls_tlssocket_getpeercertificate_detailed
 [`request.socket`]: #http_request_socket
+[`request.writableFinished`]: #http_request_writablefinished
 [`request.write(data, encoding)`]: #http_request_write_chunk_encoding_callback
 [`response.end()`]: #http_response_end_data_encoding_callback
 [`response.getHeader()`]: #http_response_getheader_name
 [`response.setHeader()`]: #http_response_setheader_name_value
 [`response.socket`]: #http_response_socket
+[`response.writableFinished`]: #http_response_writablefinished
 [`response.write()`]: #http_response_write_chunk_encoding_callback
 [`response.write(data, encoding)`]: #http_response_write_chunk_encoding_callback
 [`response.writeContinue()`]: #http_response_writecontinue
diff --git a/doc/api/http2.md b/doc/api/http2.md
index 1022eeb913563a..8a92aad421dea2 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -3271,6 +3271,17 @@ added: v8.4.0
 
 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])
 <!-- YAML
 added: v8.4.0
@@ -3510,3 +3521,4 @@ following additional properties:
 [`tls.connect()`]: tls.html#tls_tls_connect_options_callback
 [`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
 [error code]: #error_codes
+[`writable.writableFinished`]: stream.html#stream_writable_writablefinished
diff --git a/doc/api/stream.md b/doc/api/stream.md
index 07755e05dc2f65..14f5a9032eee2f 100644
--- a/doc/api/stream.md
+++ b/doc/api/stream.md
@@ -494,6 +494,17 @@ added: v11.4.0
 
 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
 <!-- YAML
 added: v12.6.0
@@ -2707,7 +2718,9 @@ contain multi-byte characters.
 [`stream.unpipe()`]: #stream_readable_unpipe_destination
 [`stream.wrap()`]: #stream_readable_wrap_stream
 [`writable.cork()`]: #stream_writable_cork
+[`writable.end()`]: #stream_writable_end_chunk_encoding_callback
 [`writable.uncork()`]: #stream_writable_uncork
+[`writable.writableFinished`]: #stream_writable_writablefinished
 [`zlib.createDeflate()`]: zlib.html#zlib_zlib_createdeflate_options
 [API for Stream Consumers]: #stream_api_for_stream_consumers
 [API for Stream Implementers]: #stream_api_for_stream_implementers
diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
index f2390a686d3690..2d2bbaa0e72e9c 100644
--- a/lib/_http_outgoing.js
+++ b/lib/_http_outgoing.js
@@ -575,6 +575,10 @@ Object.defineProperty(OutgoingMessage.prototype, 'headersSent', {
   get: function() { return !!this._header; }
 });
 
+Object.defineProperty(OutgoingMessage.prototype, 'writableEnded', {
+  get: function() { return this.finished; }
+});
+
 
 const crlf_buf = Buffer.from('\r\n');
 OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
diff --git a/lib/_stream_duplex.js b/lib/_stream_duplex.js
index d20877e921e445..b96a0439f785fa 100644
--- a/lib/_stream_duplex.js
+++ b/lib/_stream_duplex.js
@@ -74,7 +74,7 @@ Object.defineProperty(Duplex.prototype, 'writableHighWaterMark', {
   // userland will fail
   enumerable: false,
   get() {
-    return this._writableState.highWaterMark;
+    return this._writableState && this._writableState.highWaterMark;
   }
 });
 
@@ -94,7 +94,7 @@ Object.defineProperty(Duplex.prototype, 'writableLength', {
   // userland will fail
   enumerable: false,
   get() {
-    return this._writableState.length;
+    return this._writableState && this._writableState.length;
   }
 });
 
@@ -104,7 +104,17 @@ Object.defineProperty(Duplex.prototype, 'writableFinished', {
   // userland will fail
   enumerable: false,
   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;
   }
 });
 
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index 315360ca6b6ac7..47a9bb208cd5e3 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -352,13 +352,23 @@ function decodeChunk(state, chunk, encoding) {
   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', {
   // Making it explicit this property is not enumerable
   // because otherwise some prototype manipulation in
   // userland will fail
   enumerable: false,
   get: function() {
-    return this._writableState.highWaterMark;
+    return this._writableState && this._writableState.highWaterMark;
   }
 });
 
@@ -711,7 +721,7 @@ Object.defineProperty(Writable.prototype, 'writableFinished', {
   // userland will fail
   enumerable: false,
   get() {
-    return this._writableState.finished;
+    return this._writableState ? this._writableState.finished : false;
   }
 });
 
diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
index b4357058540429..9cf19250a8a3b0 100644
--- a/lib/internal/http2/compat.js
+++ b/lib/internal/http2/compat.js
@@ -454,6 +454,11 @@ class Http2ServerResponse extends Stream {
     return this.headersSent;
   }
 
+  get writableEnded() {
+    const state = this[kState];
+    return state.ending;
+  }
+
   get finished() {
     const stream = this[kStream];
     return stream.destroyed ||
diff --git a/test/parallel/test-http-outgoing-finish-writable.js b/test/parallel/test-http-outgoing-finish-writable.js
index e03901fb6ddf07..e3c870164bac58 100644
--- a/test/parallel/test-http-outgoing-finish-writable.js
+++ b/test/parallel/test-http-outgoing-finish-writable.js
@@ -9,12 +9,14 @@ const http = require('http');
 const server = http.createServer(common.mustCall(function(req, res) {
   assert.strictEqual(res.writable, true);
   assert.strictEqual(res.finished, false);
+  assert.strictEqual(res.writableEnded, false);
   res.end();
 
   // res.writable is set to false after it has finished sending
   // Ref: https://github.com/nodejs/node/issues/15029
   assert.strictEqual(res.writable, true);
   assert.strictEqual(res.finished, true);
+  assert.strictEqual(res.writableEnded, true);
 
   server.close();
 }));
diff --git a/test/parallel/test-http2-compat-serverresponse-end.js b/test/parallel/test-http2-compat-serverresponse-end.js
index e0a49ca23a45f1..5bbb24bb2edb31 100644
--- a/test/parallel/test-http2-compat-serverresponse-end.js
+++ b/test/parallel/test-http2-compat-serverresponse-end.js
@@ -150,8 +150,10 @@ const {
   // for http1 compatibility
   const server = createServer(mustCall((request, response) => {
     strictEqual(response.finished, true);
+    strictEqual(response.writableEnded, false);
     response.writeHead(HTTP_STATUS_OK, { foo: 'bar' });
     response.end('data', mustCall());
+    strictEqual(response.writableEnded, true);
   }));
   server.listen(0, mustCall(() => {
     const { port } = server.address();
diff --git a/test/parallel/test-http2-compat-serverresponse-finished.js b/test/parallel/test-http2-compat-serverresponse-finished.js
index 4da592a5b354b0..a42d40227ca33b 100644
--- a/test/parallel/test-http2-compat-serverresponse-finished.js
+++ b/test/parallel/test-http2-compat-serverresponse-finished.js
@@ -25,8 +25,10 @@ server.listen(0, common.mustCall(() => {
       }));
     }));
     assert.strictEqual(response.finished, false);
+    assert.strictEqual(response.writableEnded, false);
     response.end();
     assert.strictEqual(response.finished, true);
+    assert.strictEqual(response.writableEnded, true);
   }));
 
   const url = `http://localhost:${port}`;
diff --git a/test/parallel/test-stream-writable-ended-state.js b/test/parallel/test-stream-writable-ended-state.js
index 69ba3c88ede253..e5fa624c12aed7 100644
--- a/test/parallel/test-stream-writable-ended-state.js
+++ b/test/parallel/test-stream-writable-ended-state.js
@@ -9,13 +9,17 @@ const writable = new stream.Writable();
 
 writable._write = (chunk, encoding, cb) => {
   assert.strictEqual(writable._writableState.ended, false);
+  assert.strictEqual(writable.writableEnded, false);
   cb();
 };
 
 assert.strictEqual(writable._writableState.ended, false);
+assert.strictEqual(writable.writableEnded, false);
 
 writable.end('testing ended state', common.mustCall(() => {
   assert.strictEqual(writable._writableState.ended, true);
+  assert.strictEqual(writable.writableEnded, true);
 }));
 
 assert.strictEqual(writable._writableState.ended, true);
+assert.strictEqual(writable.writableEnded, true);