reevaluate how to set writable = false in IncomingMessage #20233

Activity

1. apapirovski commented on Apr 23, 2018

   

   apapirovski

   on Apr 23, 2018

   Last edited by apapirovski

   Contributor

   More actions

   I think it's nearly impossible without breaking a super popular module. See #15029 and #15404

   Specifically it's because end-of-stream treats OutgoingMessage as a legacy stream since it doesn't have _writableState. I don't know a great way out of it without some major breakage and a semver-major change.

   ping @mafintosh @mcollina
2. ronag commented on Apr 23, 2018

   

   ronag

   on Apr 23, 2018

   Last edited by ronag

   MemberAuthor

   More actions

   I believe one way could be into fooling eos into thinking it's a isRequest and that way it doesn't use onlegacyfinish.

   i.e. if we just add a dummy property on IncomingMessage, e.g.

   this.setHeader = 'this is a truthy dummy value for eos'

   It's a bit hacky. But I think it's better than the public API being "incorrect".
3. apapirovski commented on Apr 23, 2018

   

   apapirovski

   on Apr 23, 2018

   Contributor

   More actions

   I think we would want to avoid adding hacks like that. It's hard to know what else it could break since it would then start to resemble express (which is what that's testing).

   (The alternative along the same lines would be adding dummy writableState but it's bad for the same reason as above.)
4. ronag commented on Apr 23, 2018

   

   ronag

   on Apr 23, 2018

   Last edited by ronag

   MemberAuthor

   More actions

   How about if we ask end-of-stream to fix it in a way that is backwards compatible and then we add a check in nodejs (10) for incompatible package versions and print warnings?

   As it is now the API is a bit broken and I've seen people doing this:

   if (!res.headersSent && res.writable) {
     res.writeHead(500)
     res.end()
   } else {
     res.destroy()
   }

   We might need a strategy for packages that depend on broken behavior?
5. ronag commented on Apr 23, 2018

   

   ronag

   on Apr 23, 2018

   Last edited by ronag

   MemberAuthor

   More actions

   Alternatively we could deprecate writable and add a new property that behaves properly everywhere.
6. ronag commented on Apr 23, 2018

   

   ronag

   on Apr 23, 2018

   MemberAuthor

   More actions

   Would another idea be to implement hot patching in the module loader to re-write known broken package instances?

   Sorry, just thinking of all crazy ideas I can come up with.
7. mcollina commented on Apr 23, 2018

   

   mcollina

   on Apr 23, 2018

   SponsorMember

   More actions

   The only solution is to make OutgoingMessage a Writable. However, that causes a major performance regression (I tried that).

   It is possible to implement some fix for this, but it will take a lot of time. The generic idea is to make sure end-of-stream do not need to check for _writableState, and it uses accessor properties instead. However, we just got finished on Node 10, which should behave similarly.

   I’m -1 for the time being, the solution is going to be very hard, and it will also require time to the ecosystem to catch up.

   The good news is that we are working to improve streams, and we should be able to fix this in the future by not having userland to resort to _writableState access.

   cc @mafintosh
8. ronag

   closed this as completedon Apr 23, 2018
9. mafintosh commented on May 5, 2018

   

   mafintosh

   on May 5, 2018

   Member

   More actions

   If someone wants to improve eos to not use the legacy mode for newer http streams in Node I'd be 👍. Unsure how tricky that is when supporting older Node.js versions at the same time.
10. mafintosh commented on May 5, 2018

    

    mafintosh

    on May 5, 2018

    Member

    More actions

    A solution could be if there was a way in core streams to detect if a stream is a newer stream that didn't involve checking for _readableState etc