Repository navigation
Deprecating autoDestroy: false and autoClose: false #30621
Description
I find allowing the option of not having autoDestroy and autoClose as problematic and I'm not sure what the motivation for these options would be for "correctly" implemented streams.
Would it be an option to change the defaults for these to true and documentation deprecating them? In order to make this transition faster we should explicitly set them to false in the places in core which are not yet updated for these.
From my experience when people try to use these options it's often due not fully understanding the streams API and trying to get around that but at the same time ending up accidentally creating some very subtle bugs.
@mcollina: Thoughts?
Activity
lpinca commentedon Nov 24, 2019
It is not always trivial to implement streams and these options help. For example I use
autoDestroyandemitClosein ws https://github.com/websockets/ws/blob/7.2.0/lib/stream.js#L67-L68. Sometimes you need to change default behavior if it does not fit your needs.I'm fine with changing defaults but not deprecating the options.
mcollina commentedon Nov 24, 2019
I’m -1 in deprecating, but + 1 in getting a semver-major PR that flip those defaults.
ronag commentedon Nov 24, 2019
I'm not convinced you need those options to implement that, e.g. https://gist.github.com/ronag/3fd8012fb0b0e23114714d1629c5a0a8. Probably doesn't work but just an example.
It's probably a bit to involved of an example for me to be able to argue my point without researching a some more into it.
lpinca commentedon Nov 24, 2019
I don't know, I don't currently have the time to think about all the edge cases solved in that implementation and verify that they also work as expected in your example, but another reason for which those options are used is that they allow me to make the stream work in the same way in all supported Node.js versions without using
readable-stream.ronag commentedon Nov 24, 2019
The point here is not to re-write lots of existing code. My main point here is how to make future stream implementers not use the options unless strictly necessary (which should be very unusual).
I don't think just changing the defaults is enough to stop people from using these options as crutches. I guess an alternative to deprecation is to add explicit discouragement in the docs?
mcollina commentedon Nov 24, 2019
Let’s start with changing the defaults, I think deprecating might come 1 major after that, and we are already in Node 15 territory.
lpinca commentedon Nov 24, 2019
Maybe it's just me but I don't change defaults unless I have a good reason to do that so I don't think we should ever deprecate these options.
I mean, if I'm changing a default I know exactly what I'm doing and why I'm doing it but ofc I can't speak for other people.
ronag commentedon Nov 24, 2019
I'm not convinced. I maintain that we should look into eventually deprecating them in the future. If I encounter any good example where they are indeed required I will of course reconsider. I don't think there is any hurry to actually remove them from the code. I'm mostly interested in the docs on this specific point.
mcollina commentedon Nov 24, 2019
I think you are contradicting yourself. On one side you say we won’t be able to port http2 to autoDestroy, and on the other that you’d like to deprecate or remove that option.
IMHO flipping the default is enough, and possibly doc-deprecation would be enough.
ronag commentedon Nov 24, 2019
I said it will be difficult and I don't think we can do it for 14.
I'm only arguing for doc-deprecation at this point, so I think we are in agreement.
stream: enable autoDestroy by default
davedoesdev commentedon Jan 18, 2020
How do I check whether a stream was closed due explicit
destroy()call orautoDestroy?ronag commentedon Jan 18, 2020
You can't. Why do you want this?
davedoesdev commentedon Jan 18, 2020
For testing, where I was assuming I'd get
closeorendbut not both. It's fine, I'll share the same handler and only-once it.ronag commentedon Jan 18, 2020
This is an incorrect assumption :). You should always get
'close'.davedoesdev commentedon Jan 18, 2020
Only with #30623, right? I'm trying to get ahead of my tests failing.
ronag commentedon Jan 18, 2020
If you don't have
autoDestroy: trueyou should calldestroy()/destroy(err)yourself and thus'close'should be emitted. But yea, if you don't do that then it won't be emitted, at all.ronag commentedon Feb 9, 2020
We've changed the default to
true. I think this is mostly resolved through that.kanongil commentedon May 5, 2020
@ronag This is difficult to apply correctly to readables. If you call
destroy()afterpush(null), it will immediately mark the stream destroyed, which means that anything that is still in the internal buffers can be discarded, depending on how the stream is consumed. To make it work, you have to listen to your own'end'emit to time it.julienw commentedon Jun 4, 2020
On one hand I'm mostly supportive of the change because it makes sense, on the other hand this small change produced a subtle bug in my code, probably because I was using the stream API incorrectly :-) I wonder if that should be made somewhat more visible in a "Changes that may break your code" section in the v14 changelog.
For example I like how the folks from Flow write their changelogs (see https://github.com/facebook/flow/releases/tag/v0.125.0 as an example).