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.

Sign up for GitHub

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

Jump to bottom

stream: writableEnded #28934

Closed
wants to merge 2 commits into from
Closed

stream: writableEnded #28934

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 2, 2019 •
edited
Loading

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 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 ronag force-pushed the stream-writableEnded branch from f07dd71 to b5047fa Compare August 2, 2019 06:18
@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

ping @mcollina @benjamingr

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

mcollina commented Aug 2, 2019 •
edited
Loading

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

Good work!

@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

@mcollina Updated

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

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

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

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Some doc nits.

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http2.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-writableEnded branch 2 times, most recently from 7c29569 to 6ab4ad4 Compare August 2, 2019 16:53
trivikr
trivikr requested changes Aug 2, 2019
View reviewed changes
doc/api/http2.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@mcollina
Copy link
Member

mcollina commented Aug 3, 2019 •
edited
Loading

Would you mind rebase on top of master?

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

ronag commented Aug 3, 2019

done

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 17, 2019
@Trott
Copy link
Member

Trott commented Aug 17, 2019

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
Trott approved these changes Aug 17, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes.

@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

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
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 17, 2019

Landed in 6f613d8

@Trott Trott closed this Aug 17, 2019
Trott pushed a commit that referenced this pull request Aug 17, 2019
@ronag @Trott
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
@ronag @targos
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 targos mentioned this pull request Aug 19, 2019
Merged
targos added a commit that referenced this pull request Aug 19, 2019
@targos
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
@targos
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
@targos
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
@targos
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
@targos
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@trivikr trivikr trivikr approved these changes

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@ronag @mcollina @nodejs-github-bot @Trott @benjamingr @vsemozhetbyt @trivikr