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.

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

ReadableStream internal state #39627

Open
ronag opened this issue Aug 2, 2021 · 22 comments
Open

ReadableStream internal state #39627

ronag opened this issue Aug 2, 2021 · 22 comments

Comments

@ronag
Copy link
Member

ronag commented Aug 2, 2021

While working implementing fetch in undici I've noticed a limitation on how compliant we can be towards the spec.

In order to "extract" a request body we need to have access to the streams [[disturbed]] state. However, this state is inaccessible through the public API.

Is there a way we could make this accessible behind a public symbol or something? Otherwise fetch can't be fully implemented outside of core as per my understanding of the specification.

@ronag
Copy link
Member Author

ronag commented Aug 2, 2021

@jasnell @benjamingr

@ronag
Copy link
Member Author

ronag commented Aug 2, 2021

@nodejs/whatwg-stream

@mcollina
Copy link
Member

mcollina commented Aug 2, 2021

I'm +1 to expose this via a Symbol('nodejs.webstreams.disturbed').

@domenic wdyt?

@benjamingr
Copy link
Member

I'll reiterate:

  • We can't add properties or symbols to the stream, that would violate the spec. That point is very clear since it would hurt interop. Just like IE adding random properties to NodeList back when
  • We are welcome to add statics or anything that is part of core and has access to internal state without changing the standard interface.

This is why we have a static getEventListeners rather than modifying EventTarget for example.

I vote a static disturbed (or better an object that exposes all of these) that works on both web and Node.js streams

@mcollina
Copy link
Member

mcollina commented Aug 2, 2021

We can't add properties or symbols to the stream, that would violate the spec.

We'll end up adding something anyway as we would not have any other way to accessing this state. We might just decide to not expose this Symbol and have it in a not-enumberable property... however we'll need to have it there anyway.

@ronag
Copy link
Member Author

ronag commented Aug 2, 2021

We can't add properties or symbols to the stream, that would violate the spec.

We'll end up adding something anyway as we would not have any other way to accessing this state. We might just decide to not expose this Symbol and have it in a not-enumberable property... however we'll need to have it there anyway.

We already add symbols?

ronag added a commit to nxtedition/node that referenced this issue Aug 2, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627
ronag added a commit to nxtedition/node that referenced this issue Aug 2, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627
@benjamingr
Copy link
Member

however we'll need to have it there anyway.

Adding things we don't consider our public API is risky but fine as long as it's implementation detail and we don't rely on it. If we can't even build without the internal API - that's a good indication we should expose that API (via a static method).

Since there is a simple, spec-compliant workaround here (a static method) I suggest we stick to that :)

(If we need an API like isDisturbed - contributing back to whatwg is also a good viable option, I am super busy with baby/new-job/new-house/family-health-stuff but technically that can be something I work on in my job, just not in the coming months)

@benjamingr
Copy link
Member

whatwg/streams#1025 this looks positive

@domenic
Copy link
Contributor

domenic commented Aug 2, 2021

Adding non-spec statics is not spec-compliant; I'm not sure why you'd think they're any different from prototype properties.

@jasnell
Copy link
Member

jasnell commented Aug 2, 2021

I would add separate methods that are not hung off the classes at all.

const { isDisturbed, isFinished } = require ('stream')

isDisturbed(readable)

isFinished(readable)

The challenge with these is that they will have to rely on internal state and not public API but that's fine.

ronag added a commit to nxtedition/node that referenced this issue Aug 2, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627
@benjamingr
Copy link
Member

@domenic I thought adding statics that are not part of the spec does not violate the spec while adding methods to the interface does. Did I misunderstand?

const s = getStream();
s.isDisturbed(); // not allowed, adds a method to the prototype
ReadableStream.isDisturbed(s); // not allowed, adds a method to ReadableStream
const { isDisturbed } = await import('nodejs/stream-utils');
isDisturbed(s); // this is fine

@benjamingr
Copy link
Member

Or did my phrasing imply I meant adding a static to the interface? (That was not my intention nor the implementation in the PR ronag followed up with)

@domenic
Copy link
Contributor

domenic commented Aug 3, 2021

Ah, yeah, I thought by static you meant the thing you get in JavaScript when using the static keyword :). If you're talking about module exports then no problem!

ronag added a commit that referenced this issue Aug 6, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: #39627

PR-URL: #39628
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@ronag ronag closed this as completed Aug 7, 2021
@ronag ronag reopened this Aug 8, 2021
@ronag
Copy link
Member Author

ronag commented Aug 8, 2021

I have to re-open this as I've encountered another state.

https://fetch.spec.whatwg.org/#abort-fetch

If response’s body is not null and is readable, then error response’s body with error.

How can I inspect whether "body" is "readable" without access to internal state?

A ReadableStream stream is readable if stream.[[state]] is "readable".

@benjamingr @domenic @jasnell

@ronag
Copy link
Member Author

ronag commented Aug 11, 2021

@jasnell I don't think you are going to be happy with me... but I found a way 😄

function isReadable (stream) {
  // This is a hack!
  return /state: 'readable'/.test(util.inspect(stream))
}

@benjamingr
Copy link
Member

@ronag

My understanding is - fetch is going to be part of Node, web streams are native to Node. This sort of thing can be a problem if we want to make it possible to implement fetch on top of web streams in user-land. Do I understand incorrectly?

@ronag
Copy link
Member Author

ronag commented Aug 11, 2021

Correct. We need it to implement fetch in user land outside of core.

@domenic
Copy link
Contributor

domenic commented Aug 11, 2021

If async suffices the following should work I believe:

async function isReadable(reader) {
  let readable = true;
  reader.closed.then(() => { readable = false; }, () => { readable = false; });
  await Promise.resolve();

  // If the stream was closed or errored then the closed promise would have settled by now and updated readable.
  // If not then it's readable.
  return readable;
}

This is untested. In particular you might need more than one await Promise.resolve() in case the process of signaling the closed promise goes through a number of promise chains. await new Promise(setTimeout) should definitely work.

@jasnell
Copy link
Member

jasnell commented Aug 11, 2021

@ronag:

I don't think you are going to be happy with me... but I found a way

You're right! Don't do that lol.

@domenic ... one challenge we're going to have here is that we need to be able to find out the readable/locked/closed/errored state when we likely won't have access to any attached reader.

I've been thinking about opening a spec proposal to add a couple of new getters to the ReadableStream and WritableStream classes. Specifically, stream.locked and stream.status, both of which would be read-only. Before I do so, however, it would be great to get your opinion on it.

@ronag
Copy link
Member Author

ronag commented Aug 11, 2021

Stream.locked already exists, no?

@domenic
Copy link
Contributor

domenic commented Aug 11, 2021

Well, only the person with the reader should be able to introspect the stream; that's a design goal.

ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627

PR-URL: nodejs#39628
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627

PR-URL: nodejs#39628
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Backport-PR-URL: nodejs#39819
ronag added a commit to nxtedition/node that referenced this issue Aug 20, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: nodejs#39627

PR-URL: nodejs#39628
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Backport-PR-URL: nodejs#39819
targos pushed a commit that referenced this issue Aug 23, 2021
Adds a helper util used to determine whether a stream has been
disturbed (read or cancelled).

Refs: #39627

PR-URL: #39628
Backport-PR-URL: #39819
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@rumkin
Copy link
Contributor

rumkin commented Oct 18, 2021

What's about @domenic's example:

async function isReadable(reader) {
  let readable = true;
  reader.closed.then(() => { readable = false; }, () => { readable = false; });
  await Promise.resolve();

  // If the stream was closed or errored then the closed promise would have settled by now and updated readable.
  // If not then it's readable.
  return readable;
}

I'll copy my opinion over it from whatwg/streams#1025:

It creates 3 promises and 3 jumps over event loop stack just to determine stream state. Moreover function scope with its boolean, function and one of this promises will live as long as stream is not closed and thus would hold memory. In the case of high loads, this will be memory pressure out of nowhere. This check should be made in a single synchronous call.

So having synchronous check in the case of directly owned a) non-nested undisturbed stream and b) disturbed or closed streams (I expect it as the most cases) all of extra payload will be eliminated, and only indirectly owned undisturbed or closing nested streams would require read attempt to fail. Thus provided code is doing redundant work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants