fix(ext/fetch): handle errors in req body stream #17081
Conversation
Right now an error in a request body stream causes an uncatchable global promise rejection. This PR fixes this to instead propagate the error correctly into the promise returned from `fetch`. It additionally fixes errored readable stream bodies being treated as successfully completed bodies by Rust.
ext/fetch/lib.rs
Outdated
| Box::pin(async move { | ||
| let body = RcRef::map(&self, |r| &r.body).borrow_mut().await; | ||
| let cancel = RcRef::map(self, |r| &r.cancel); | ||
| body.send(None).or_cancel(cancel).await?.ok(); // we don't care if the receiver is closed, because this is the shutdown |
There was a problem hiding this comment.
I would probably not ignore this. If the receiver left before receiving the EOF it is still a failure.
There was a problem hiding this comment.
Not always - there is one case where hyper knows the size of the response body up front (through content-length header on the resp), where it will drop the body once that content length has been reached, regardless of if the stream is complete or not. This is expected behaviour, but it means that if you stream a body with an up front known size (eg a Blob), shutdown will never succeed because the body (and by extension the receiver) will have dropped by the time we try to shutdown.
I could try to wire it up to only swallow the error if this specific case has happened (might be a bit tricky to detect though).
There was a problem hiding this comment.
That's not correct. You can see this in action using this playground: https://dash.deno.com/playground/slow-sheep-54
TL;DR: Content-Length too small results in an errored response stream.
🦕 curl https://slow-sheep-54.deno.dev/ # No Content-Length
Hello World%
🦕 curl https://slow-sheep-54.deno.dev/11 # Content-Length accurate.
Hello World%
🦕 curl https://slow-sheep-54.deno.dev/100 # Content-Length larger than response.
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
🦕 curl https://slow-sheep-54.deno.dev/1 # Content-Length smaller than response.
curl: (92) HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)
| done = res.done; | ||
| val = res.value; | ||
| } catch (err) { | ||
| if (terminator.aborted) break; |
There was a problem hiding this comment.
You don't want to send an error to the response stream if terminated?
There was a problem hiding this comment.
Yes I do - if terminator.aborted is set, core.shutdown won't be called further down in the code. It uses a different error throwing path though.
There was a problem hiding this comment.
Consider using the same error paths, multiple paths lead to mistakes and difficult-to-understand code.
There was a problem hiding this comment.
Yeah, I want to refactor the terminator code paths in a follow up.
| @@ -200,6 +200,8 @@ | |||
| } | |||
| terminator[abortSignal.add](onAbort); | |||
|
|
|||
| let requestSendError; | |||
| let requestSendErrorSet = false; | |||
There was a problem hiding this comment.
So cumbersome to use 2 variables for this.
If you're concerned that someone might throw undefined you could use a magic value (e.g. A symbol) to indicate "no error".
| resp = await opFetchSend(requestRid); | ||
| } catch (err) { | ||
| if (terminator.aborted) return; | ||
| if (requestSendErrorSet) { |
There was a problem hiding this comment.
So requestSendError gets ignored if opFetchSend() succeeds?
Help me understand why.
There was a problem hiding this comment.
Yes - there is a follow up to be done here that propagates this error into the resp body ReadableStream - but I haven't quite figured out how to wire that up. This is what the TODO I added is about.
Right now an error in a request body stream causes an uncatchable global promise rejection. This PR fixes this to instead propagate the error correctly into the promise returned from `fetch`. It additionally fixes errored readable stream bodies being treated as successfully completed bodies by Rust.
Right now an error in a request body stream causes an uncatchable
global promise rejection. This PR fixes this to instead propagate the
error correctly into the promise returned from
fetch.It additionally fixes errored readable stream bodies being treated as
successfully completed bodies by Rust.