Skip to content

http: align OutgoingMessage.writable with streams#33655

Closed
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:outgoing-writable
Closed

http: align OutgoingMessage.writable with streams#33655
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:outgoing-writable

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 addaleax and mcollina 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

mcollina commented May 30, 2020

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

addaleax commented May 30, 2020

@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 force-pushed the outgoing-writable branch from 1bff16b to 0f7b51b Compare June 14, 2020 17:40
ronag
ronag commented Jun 14, 2020
View reviewed changes
test/parallel/test-http-writable-true-after-close.js
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

@ronag ronag Jun 14, 2020

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

nodejs-github-bot commented Jun 15, 2020

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