ReadableStream appears incompatible with Sync XHR
gterzian opened this issue · 9 comments
Looking at Step 12 of https://xhr.spec.whatwg.org/#the-send()-method.
So at step 12.2, I think the task on the event-loop from where the call originates "blocks" on the "fetch" call returning a response.
Then the problem I have run into is that as part of fetch, we have to transmit the request body, which I think requires running code on the event-loop(although that is actually a question I've asked at whatwg/fetch#976).
So in other words, in order to transmit the request body of the fetch, we need to read from the stream, which, at least if the body is a user provided stream, will require running code on an event-loop. It that event-loop is the same that is currently blocking on the fetch call as part of a sync XHR, it appears to be an actual deadlock.
I'm actually running into this very problem while integrating streams into the Servo project(see servo/servo#25873 (comment)).
One potential solution, both at the level of the spec and as a practical solution, could be to add a new step prior to step 12.2(the fetch call), where one would acquire a reader, and then read all the chunks from the stream, and then use the bytes directly to transmit the body as part of the fetch.
Although I think this could raise some issues because reading chunks from the stream could potentially result in the user stream code running, which would mean the XHR wouldn't be really "sync" anymore.
I see two solutions I like a bit better as I don't really care for XMLHttpRequest:
- We use a more limited form of BodyInit for XMLHttpRequest
- We throw when you pass a ReadableStream and the synchronous flag is set.
Note that upload streams in general still have issues that are being sorted out in the Fetch repository.
So in other words, in order to transmit the request body of the fetch, we need to read from the stream, which, at least if the body is a user provided stream, will require running code on an event-loop. It that event-loop is the same that is currently blocking on the fetch call as part of a sync XHR, it appears to be an actual deadlock.
I'm confused by this paragraph, as for XHR there is no user provided stream.
@yutakahirano there is, send() takes BodyInit which consists of ReadableStream among other things.
Oh sorry I missed that, thank you. Then I prefer removing that functionality from XHR. I'm not sure if which of throwing or ignoring is better.
Makes sense, strictly speaking even in the other BodyInit cases those objects are extracted into a stream, and those could be said to be fully "native" streams where reading chunks would not require calling into JS.
How about we remove the functionality, and then implementations can still warn in their own way if needed about use of a deprecated feature?
I can open a PR...
On the other hand, returning an error in the stream case allows us to still use BodyInit, versus adding a new IDL like XhrBodyInit that would not include a stream.
But send doesn't return an error now, so that's maybe another complication.
We could also define send like:
void send(optional (Document or Blob or BufferSource or FormData or URLSearchParams or USVString)? body = null);
The algorithm would still "work", since "Otherwise, set request body and extractedContentType to the result of extracting body" would work "as is" I think.
Thinking about it more I suspect implementations don't support this today and what happens is that ReadableStream gets stringified. That's acceptable to me.
I would suggest we define XMLHttpRequestBodyInit in Fetch and define BodyInit as (ReadableStream or XMLHttpRequestBodyInit). And make XMLHttpRequest use (Document or XMLHttpRequestBodyInit).
ReadableStream will stringify in IDL land and then be treated as a string in XMLHttpRequest as a result.
If you're willing to tackle this @gterzian that'd be great!
Ok I'll take it on, starting with the change in Fetch then.