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

http: align OutgoingMessage.writable with streams #33655

Closed
wants to merge 2 commits into from
Closed

http: align OutgoingMessage.writable with streams #33655

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented May 30, 2020 •
edited
Loading

Should be a computed value that looks at finished and
destroyed state.

Fixes: #33643

Please keep this in mind when approving this PR.

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

@ronag ronag added the http Issues or PRs related to the http subsystem. label May 30, 2020
@ronag ronag force-pushed the outgoing-writable branch 3 times, most recently from 43d902a to dfe1c66 Compare May 30, 2020 08:55
@ronag ronag mentioned this pull request May 30, 2020
Closed
@ronag ronag requested review from mcollina and addaleax and removed request for mcollina May 30, 2020 08:55
@ronag ronag force-pushed the outgoing-writable branch 2 times, most recently from efba06d to 1bff16b Compare May 30, 2020 09:51
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2020
@mcollina
Copy link
Member

I have a feeling this could be significantly breaking. I thought we tried to do this in the past and we had to revert.

@addaleax
Copy link
Member

@mcollina Did you see #33591 (comment)? Currently on Node.js 14 you need to check whether an OutgoingMessage is writable or not before writing to it, and it is odd that you can only do that through .destroyed and not .writable.

@jasnell
Copy link
Member

jasnell commented May 30, 2020

Yeah, it would be fantastic to get this fixed but it definitely can be breaking.

/cc @nodejs/web-server-frameworks folks

@targos
Copy link
Member

targos commented May 30, 2020

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2388/

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@mcollina
Copy link
Member

mcollina commented Jun 1, 2020

I might be ok to add this as a fix, but I would be way more comfortable in reverting #33131 in v14 instead of also landing this. We might risk to break things even more.

Then we should land both these changes in master.

@ronag
Copy link
Member Author

ronag commented Jun 1, 2020 •
edited
Loading

I might be ok to add this as a fix, but I would be way more comfortable in reverting #33131 in v14 instead of also landing this. We might risk to break things even more.

Sounds reasonable to me.

EDIT: #33686

@ronag ronag mentioned this pull request Jun 1, 2020
Closed
4 tasks
@ronag
http: align OutgoingMessage.writable with streams
7cbc07f 
Should be a computed value that looks at finished and
destroyed state.

Fixes: nodejs#33643
@ronag
Copy link
Member Author

ronag commented Jun 14, 2020

Please keep this in mind when approving this PR.

ronag
ronag commented Jun 14, 2020
View reviewed changes
test/parallel/test-http-writable-true-after-close.js
@@ -4,16 +4,13 @@ const common = require('../common');
const assert = require('assert');
const { get, createServer } = require('http');

// res.writable should not be set to false after it has finished sending
// Ref: https://github.com/nodejs/node/issues/15029
Copy link
Member Author

Choose a reason for hiding this comment

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

Please read through the issue to make sure you agree with the consequences of this change.

@ronag ronag requested a review from jasnell June 14, 2020 17:41
@ronag
Copy link
Member Author

ronag commented Jun 14, 2020

@nodejs/http @nodejs/web-server-frameworks

@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina requested changes Jun 15, 2020
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.

I'm not really convinced as I think the problems outlined in #15029 would represent themselves again.

@ronag ronag added the blocked PRs that are blocked by other issues or PRs. label Jun 15, 2020
@ronag
Copy link
Member Author

ronag commented Jun 15, 2020 •
edited
Loading

Labelling this as blocked until I have time to dig further into the referenced issue.

This was referenced Jun 18, 2020
Open
Open
Open
@ronag ronag mentioned this pull request Jun 21, 2020
Closed
4 tasks
@ronag ronag requested a review from a team as a code owner August 10, 2020 16:07
@ronag ronag closed this Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina requested changes

@addaleax addaleax Awaiting requested review from addaleax

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OutgoingMessage should set writable false
6 participants
@ronag @mcollina @addaleax @jasnell @targos @nodejs-github-bot