stream: deprecate writable/readable

[ronag]

• Doc deprecate writable/readable
• Remove usage of writable/readable from core. Leave it just for user land compat.

EDIT: Updated description. Old description remains below.

I think there is a potential issue with `'end'` not being emitted despite the `IncomingMessage` being `readable`. We've encountered this here https://github.com//pull/29376. There are fixes pending (https://github.com//pull/27984) for the observed behaviour of this (at least in terms of emitted events). But I think it might be worth investigating why `'end'` is not emitted despite readable being true.

fastify/fastify#1833 is also relevant.

[ronag]

@mcollina What is the expected behaviour of Stream.readable? As far as I can see it only becomes false after 'end' in Readable. So we can't actually use it like we do in ClientRequest to check whether 'end' will be emitted since it is not set to false on 'aborted', 'error' or 'close'.

The docs says:

Is true if it is safe to call [readable.read()][stream-read].

I'm not sure whether this is actually true in practice and what "safe to call" means. But it does make me contemplative about how readable is used in ClientRequest.

[ImHype]

if the socket.readable is changed to false, the end event will be emit immedately

[mcollina]

@mcollina What is the expected behaviour of Stream.readable? As far as I can see it only becomes false after 'end' in Readable.

I've never understood stream.readable or stream.writable as they are relics of stream1 day. I think it is impossible to represent the state of a stream with a boolean. I'm not a user of these myself because of this reason, and I would just doc-deprecate both stream.readable and stream.writable, and maybe fully remove them one day.

[ronag]

Ok. I'll look into doc deprecating and avoid its usage in core once the number of PRs go down.

[mscdex]

The properties are useful for immediately determining the possibility of reading/writing from/to a stream. If you're the one creating the streams, sure, you can listen for events. However if you're writing a third party module that accepts streams, you have no idea if you were just passed a stream that is not usable in its current state.

[ronag]

determining the possibility of reading/writing from/to a stream

I guess that's the theory... but I don't think it actually works like that in practice, e.g. a Readable stream is only readable=false if 'end' is emitted. Not if it has 'error'ed or 'aborted' .

Also I think (if I remember correctly) there are some possible inconsistencies in regards to readable and writable in both fs and net.

[ronag]

I guess the question is... should we deprecate, document or fix?

[ronag]

determining the possibility of reading/writing from/to a stream

Shouldn't a stream always be writable(maybe even readable) unless it is destroyed?

[mscdex]

Shouldn't a stream always be writable(maybe even readable) unless it is destroyed?

I disagree. For example, if I have a network protocol module that accepts a user-supplied net.Socket, I need to know if I can reasonably expect to get data out of the socket at any point without the user doing anything to the socket. So if the socket is closed or in the process of closing and is not currently in the process of connecting, I can expect that the socket is not going to be of use to me, so I can report an error back to the caller.

[ronag]

I'm having trouble following that reasoning. What does it have to do with writable/readable? Why won't you just have write return false to signal that it doesn't want any data? Also if the socket is closed we are already supposed to throw ERR_STREAM_DESTROYED?

[mscdex]

What does it have to do with writable/readable?

If I'm handed an arbitrary stream, I want to know immediately if calling .read() could ever result in a 'readable' event or if I will ever see a 'data' event. Being able to check .readable should help me determine that.

Why won't you just have write return false to signal that it doesn't want any data?

I'm not sure what you mean by this. From a stream consumer perspective, I'm not in control of that. From a stream creator perspective, with .write() returning false, there's no way to differentiate between say "hey, i can't accept any more data because i've reached the highWaterMark" and "hey, i can't accept any data because the underlying resource is gone."

Also if the socket is closed we are already supposed to throw ERR_STREAM_DESTROYED?

Here are a few issues with a 'destroyed' stream:

• The throwing behavior you mentioned exists only in stream.Writable
• The destroyed status is not managed by stream.Writable/stream.Readable itself, it's up to the stream implementer to set it appropriately
• Some stream implementations can "undestroy" (e.g. net.Socket), meaning they can be reused. The problem again is that this is not a part of stream.Writable/stream.Readable itself. It'd be important to ensure that streams that "undestroy" themselves reset their readable/writable` statuses appropriately.

[ronag]

This is way over my head. I'll defer to your expertise. At some point it would be nice to have this described in the documentation at the least.

Anyhow, I believe the way ClientRequest is using .readable is both unnecessary and wrong. I'll make a PR at some point with a suggestion to resolve that specifically.

[ronag]

If I'm handed an arbitrary stream, I want to know immediately if calling .read() could ever result in a 'readable' event or if I will ever see a 'data' event. Being able to check .readable should help me determine that.

In this case the stream should already be destroyed.

The throwing behavior you mentioned exists only in stream.Writable

True, but not sure if relevant.

The destroyed status is not managed by stream.Writable/stream.Readable itself, it's up to the stream implementer to set it appropriately

Not true. This is handled by the stream.Writable/stream.Readable.

Some stream implementations can "undestroy" (e.g. net.Socket), meaning they can be reused. The problem again is that this is not a part of stream.Writable/stream.Readable itself. It'd be important to ensure that streams that "undestroy" themselves reset their readable/writable` statuses appropriately.

"undestroy" is a weird quirk we should probably deprecate. It's only used by net even there it is probably subtly broken. I believe the only reason it remains is due to backwards compat.

I feel you might be right though about readable/writable in the case of Duplex and half-open streams.

[ronag]

@mscdex: I think I feel a bit more confident with streams at this point and wouldn't mind continuing this discussion if you are still up for it.

[ronag]

I think we might want to keep readable/writable because of the Duplex + half open case. But instead of being a getter/setter I think it should be a computed property, e.g.

defineProperty(Writable.prototype, 'writable', {
  get() {
     const w = this._writableState;
     if (w && typeof w[kWritable] === 'boolean') return w[kWritable];
     return w && !w.errored && !w.destroyed && !w.ended;
  },
  set(val) {
     // Backwards compat.
     const w = this._writableState;
     w[kWritable] = val;
  }
});

defineProperty(Readable.prototype, 'readable', {
  get() {
     const r = this._readableState;
     if (r && typeof r[kReadable] === 'boolean') return r[kReadable];
     return r && !r.errored && !r.destroyed && !r.ended;
  },
  set(val) {
     // Backwards compat.
     const r = this._readableState;
     r[kReadable] = val;
  }
});

Or something along those lines. This would be make these much more consistent and easier to reason about what they mean and what they do.

@mcollina: What do you think? Does something like that make sense at least in theory?

@jasnell: I noticed you were using writable & readable quite recently in the Quic fork. Maybe you have some input?

[mcollina]

We can’t remove the setter fir backward compat. I like the idea of the getter.