[Flight Reply] Don't create temporary references for objects whose parts were consumed during serialization #32702
Conversation
lubieowoce
force-pushed
the
avoid-temporary-references-with-consumed-parts
branch
from
be606fc to
c83db4e
Compare
lubieowoce
force-pushed
the
avoid-temporary-references-with-consumed-parts
branch
from
c83db4e to
ab472d7
Compare
lubieowoce
changed the title
There was a problem hiding this comment.
When I see something that leaks to the parent scope like this and trying to fix it with some stack context, that's a red flag to me. Because I've hit similar scenario in the past and it turns out to not be sufficient. Mainly because a tree isn't just one model. A model can contain references inside of it and some of those references may even be async.
For example, what happens if it's:
{ wrapper: Promise.resolve(iterator) }
We don't eagerly parse inside Promises but do that lazily. So we don't know what the deeply nested stuff will contain while parsing the wrapper. In fact, it might not even have streamed in yet.
In fact, ideally you should be able to return the temporary reference of the wrapper back to the client before even the iterator has been serialized on the client because it's still serializing. Such as if the Promise isn't resolved yet. Currently we block until the whole thing gets serialized but that's mainly historic because Safari didn't support streaming request bodies with fetch.
|
Even a partial solution might not be so great because it depends on implementation details and all data structures in between. So even if we make it work in some cases, there's a risk of us changing implementation details or the user deep down the line being able to change unrelated code along the path. That might make it not worth having a partial solution. |
|
It's interesting that this also requires deduping. It's a good reminder that deduping isn't just about identity preservation and performance but it's also required to serialize a single one-shot iterator in multiple places. We don't have perfect deduping today but that's the goal at least. |
This PR fixes a bug with passing "consumable" values from the client to the server and back. Consumable values include Iterators, AsyncIterators, and ReadableStreams -- "one-shot" values that we can only iterate over once.
The problem occurs if we attempt to roundtrip an object like
{ wrapper: iterator }.When decoding the reply, we'd currently create a temporary reference for the object, and since in the server is passing it back unchanged, we'd just return a temporary reference instead of serializing again.
However, the iterator was already consumed when the client was encoding the reply, so the final response would contain a non-usable, exhausted iterator. Same goes for AsyncIterators and ReadableStreams (which would throw when read instead of silently being done).
This PR fixes that by making
reviveModelcheck if an object contains parts that were already consumed during serialization. If it does, we skip the creation of a temporary reference for the object itself. This propagates outwards:{ outer: { inner: iterator }}should also not be returned as a temporary reference for the same reasons.Then, when serializing the response, we won't have a temporary reference for the object(s), so we'll re-serialize them and the deserialized iterator, and thus give the client a usable value.
Note that roundtripping a plain iterator (without any wrapper objects around it) is not affected by this bug, because we don't currently create temporary references for values with special decoding logic (i.e. ones returned by
parseModelString), only for plain objects / arrays, so the value would already always be re-serialized. I added tests for this as well just to make sure.