Fix bugs in handling multiple incoming bodies piped through a TransFormStream to an outgoing body
#171
Conversation
…ect handles It was pretty hard to follow when a caller would expect to have a valid handle, or when a `nullptr` would be acceptable. Signed-off-by: Till Schneidereit <[email protected]>
…ormStream` to an outgoing body This was pretty gnarly to work through, but I think came out reasonably clean. The two key pieces are: - delaying the first read from an incoming stream that's been piped to a `TransformStream` until the latter is actually read from, and hence doesn't have backpressure applied anymore. This part guarantees that when the read would happen, we have the full pipeline in place and can hand things off to the host API instead of handling anything in the JS `fetch` API implementation. - properly integrating these two pieces, and only closing the incoming body's `ReadableStream` once the host API reports that it's been dealt with to completion. Signed-off-by: Till Schneidereit <[email protected]>
Before, any chunk would be written synchronously, with write operations to the outgoing body in a loop. That both meant that large chunks would block all script execution until they're done, and that things wouldn't necessarily always work. To wit, the test added here doesn't pass without this change. It streams a sentence in a very convoluted way, sending nested requests for each word, which then respond by sending the word back, letter for letter. Without this patch, that part breaks after the second letter in a word has been sent back. Signed-off-by: Till Schneidereit <[email protected]>
There was a problem hiding this comment.
Looks great thanks for working on this. It's just a shame Fastly can't share the implementation of these improvements.
I was a little surprised the WPT had no coverage here to change, and tested the most recent WPT against this, but it seems that's just the way it goes.
Signed-off-by: Till Schneidereit <[email protected]>
Fastly doesn't need this, as C@E has a hostcall for doing this kind of append natively and much more efficiently, which the JS runtime makes use of. I'm working on adding an equivalent thing to
Same: I would've expected this to be covered somewhere and was surprised it's not. |
This was pretty gnarly to work through, but I think came out reasonably clean. The two key pieces are:
TransformStreamuntil the latter is actually read from, and hence doesn't have backpressure applied anymore. This part guarantees that when the read would happen, we have the full pipeline in place and can hand things off to the host API instead of handling anything in the JSfetchAPI implementation.ReadableStreamonce the host API reports that it's been dealt with to completion.This PR additionally contains some fixes to how writes to bodies are handled, and some cleanup. These are both in separate patches which can be reviewed separately, if that seems easier.