FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: writableEnded by ronag · Pull Request #28934 · nodejs/node · GitHub

/ node Public

stream: writableEnded - #28934

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:stream-writableEnded
ConversationCommits (2)ChecksFiles changed
Closed

stream: writableEnded#28934
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:stream-writableEnded

Conversation

ronag commented Aug 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. stream Issues and PRs related to the stream subsystem. labels Aug 2, 2019
ronag force-pushed the stream-writableEnded branch from f07dd71 to b5047fa Compare August 2, 2019 06:18

ronag commented Aug 2, 2019

Copy link
Copy Markdown
Member Author

ping @mcollina @benjamingr

ronag force-pushed the stream-writableEnded branch 2 times, most recently from 9781b2e to 2acbd15 Compare August 2, 2019 06:24
ronag mentioned this pull request Aug 2, 2019
Closed
4 tasks
ronag changed the title Stream writable ended stream: writableEnded Aug 2, 2019

mcollina commented Aug 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Could you add some text that shows the relation between this and writableFinished?

Good work!

ronag commented Aug 2, 2019

Copy link
Copy Markdown
Member Author

@mcollina Updated

ronag force-pushed the stream-writableEnded branch from 2acbd15 to d4dfee9 Compare August 2, 2019 07:11
mcollina approved these changes Aug 2, 2019
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

ronag force-pushed the stream-writableEnded branch 3 times, most recently from a6b8aa1 to 80afc1e Compare August 2, 2019 09:22
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes

vsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some doc nits.

Comment thread doc/api/stream.md Outdated
Comment thread doc/api/stream.md Outdated
Comment thread doc/api/stream.md Outdated
Comment thread doc/api/http2.md Outdated
Comment thread doc/api/http2.md Outdated
Comment thread doc/api/http2.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
ronag force-pushed the stream-writableEnded branch 2 times, most recently from 7c29569 to 6ab4ad4 Compare August 2, 2019 16:53
trivikr requested changes Aug 2, 2019
View reviewed changes
Comment thread doc/api/http2.md Outdated

nodejs-github-bot commented Aug 3, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24835/

mcollina commented Aug 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Would you mind rebase on top of master?

ronag force-pushed the stream-writableEnded branch 3 times, most recently from 2ad49f1 to 3c8930d Compare August 3, 2019 19:37
ronag force-pushed the stream-writableEnded branch from 3c8930d to 9dd1575 Compare August 3, 2019 19:39

ronag commented Aug 3, 2019

Copy link
Copy Markdown
Member Author

done

btw, what's up with the defineProperty configs that seem to be a bit all over?

Trott commented Aug 17, 2019

Copy link
Copy Markdown
Member

A request: Your PR descriptions are usually very good, but your commit messages are usually very terse. I invariably end up amending the commit message using a bunch of stuff from the PR description. If you could be descriptive in the commit messages (or at least the first commit message--subsequent fixup commits get squashed away anyway), that would make landing a little bit easier. If you have a good reason why your commit messages are terse, that's fine, I can keep updating the commit messages upon landing. But if it's all the same to you, more info in the commit messages please! (Putting this publicly because you're not the only person for whom this is true so maybe others will see it and respond accordingly. Not trying to shame anyone or anything. I've certainly had more than my own share of commit messages that didn't even contain a verb.)

Trott approved these changes Aug 17, 2019
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if CI passes.

ronag commented Aug 17, 2019

Copy link
Copy Markdown
Member Author

A request:

... I have no reason other than that's how I'm used to do it :). I'll try to improve in future PR's & commits.

nodejs-github-bot commented Aug 17, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25045/

Trott commented Aug 17, 2019

Copy link
Copy Markdown
Member

Landed in 6f613d8

Trott closed this Aug 17, 2019
Trott pushed a commit that referenced this pull request Aug 17, 2019
http,stream: add writableEnded
6f613d8 
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>
targos pushed a commit that referenced this pull request Aug 19, 2019
http,stream: add writableEnded
f9b61d2 
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>
targos mentioned this pull request Aug 19, 2019
Merged
targos added a commit that referenced this pull request Aug 19, 2019
2019-08-20, Version 12.9.0 (Current)
a87f0e1 
Notable changes:

* **crypto**:
  * Added an oaepHash option to asymmetric encryption which allows users to specify a hash function when using OAEP padding (Tobias Nießen) [#28335](#28335).
* **deps**:
  * Updated V8 to 7.6.303.29 (Michaël Zasso) [#28955](#28955).
    * Improves the performance of various APIs such as `JSON.parse` and methods
      called on frozen arrays.
    * Adds the [`Promise.allSettled`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled) method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0 (cjihrig) [#29070](#29070).
    * `UV_FS_O_FILEMAP` has been added for faster access to memory mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been added.
* **fs**:
  * Added `fs.writev` and `fs.writevSync` methods. They allow to write an array of `ArrayBufferView`s to a file descriptor (Anas Aboureada) [#25925](#25925).
* **http**:
  * Added three properties to `OutgoingMessage.prototype`: `writableObjectMode`, `writableLength` and `writableHighWaterMark` [#29018](#29018).
* **stream**:
  * Added an new property `writableEnded` to writable streams. Its value is set to `true` after `writable.end()` has been called. (Robert Nagy) [#28934](#28934).

PR-URL: #29210
targos added a commit that referenced this pull request Aug 20, 2019
2019-08-20, Version 12.9.0 (Current)
43d6619 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
targos added a commit to targos/node that referenced this pull request Aug 20, 2019
2019-08-20, Version 12.9.0 (Current)
a9adfdb 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    nodejs#28335
* deps:
  * Updated V8 to 7.6.303.29. nodejs#28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. nodejs#29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. nodejs#25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    nodejs#29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    nodejs#28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    nodejs#28934

PR-URL: nodejs#29210
targos added a commit that referenced this pull request Aug 20, 2019
2019-08-20, Version 12.9.0 (Current)
e6872f5 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
targos added a commit that referenced this pull request Aug 20, 2019
2019-08-20, Version 12.9.0 (Current)
edc83a9 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
Closed
Closed
coderabbitai Bot mentioned this pull request Jul 23, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
Trott Trott approved these changes
benjamingr benjamingr approved these changes
trivikr trivikr approved these changes
+1 more reviewer
vsemozhetbyt vsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL