Repository navigation
[BY-3684]: surface and retry lost response bodies - #53
svargas-dev wants to merge 5 commits into
Conversation
commit: |
| } catch (e) { | ||
| // @ts-ignore | ||
| console.error('Unable to parse response json', e.message) | ||
| throw new BodyReadError(e) |
There was a problem hiding this comment.
This wraps every r.json() failure, including a SyntaxError from a complete but non-JSON 200 (say a proxy's HTML page during a redeploy) and an AbortError if the caller cancelled. Retrying either of those just repeats the same result: I ran it and a GET returning HTML makes two calls, and an abort during the body read also retries, which goes against the README's "timeouts aren't retried". Could we let SyntaxError and AbortError/TimeoutError through as they are, and only wrap actual transport failures?
There was a problem hiding this comment.
Done. Only a TypeError from r.json() becomes a BodyReadError now, which is what Node, Chromium and WebKit all throw when the body is cut off. Invalid JSON, aborts and timeouts reach the handlers as they are and aren't retried.
| } catch (e) { | ||
| // @ts-ignore | ||
| console.error('Unable to parse response json', e.message) | ||
| throw new BodyReadError(e) |
There was a problem hiding this comment.
Behaviour change to be aware of: a 200 with an empty body and no content-length used to resolve fine, and now it reaches the error handler. With content-length: 0 it's still fine. I couldn't find an API route that sends that, but it's worth checking, because dashboard's surfaceError default would turn it into a 500.
| { | ||
| "name": "@beyonk/http", | ||
| "version": "13.0.0", | ||
| "version": "13.0.1", |
There was a problem hiding this comment.
Master is already on 13.0.1, which was published on 8 October without this change, so this needs to be 13.0.2.
There was a problem hiding this comment.
I've dropped the version change from the PR instead. Releases get their version in a separate commit on master, so this can go out as 13.0.2 that way.
| }) | ||
| }) | ||
|
|
||
| context('body read failure', () => { |
There was a problem hiding this comment.
Could this block use this. for its setup like the retry block below, instead of the floating lets? And could each it have one expect? "routes the failure to the default handler" and "logs the failure through the context logger" check several things at once.
There was a problem hiding this comment.
Done. Both blocks use this. for setup now, with one expect per test.
A 2xx response whose body failed to download or parse was logged and swallowed, so send() resolved undefined and callers crashed on destructure (MARKETING-APP-K). Surface it as a BodyReadError carrying the underlying cause, so request-level and global error handlers run like they do for every other failure.
Browser body truncations (TypeError: Failed to fetch) carry no error code, so the configured errors list never matched them and a lost response body still surfaced as an error. A request whose body was cut after the headers were received is safe to repeat for idempotent methods, so retry it within the existing attempts budget regardless of the errors list.
… safe methods only
9949ba4 to
25e2b6d
Compare
svargas-dev
left a comment
svargas-dev
left a comment
There was a problem hiding this comment.
Looks good to me. Since I opened the original PR, I can't approve. This could do with another set of eyes anyway, though, to be on the safe side
| /** Response body lost mid-download */ | ||
| class BodyReadError extends HttpError { | ||
| /** | ||
| * Create a new body read error | ||
| * @param {Error} cause - The error thrown while reading the response body | ||
| */ |
There was a problem hiding this comment.
Given your Ant reviewer, I would have thought it would have picked up on the comments!
| /** Response body lost mid-download */ | |
| class BodyReadError extends HttpError { | |
| /** | |
| * Create a new body read error | |
| * @param {Error} cause - The error thrown while reading the response body | |
| */ | |
| class BodyReadError extends HttpError { | |
| /** | |
| * @param {Error} cause | |
| */ |
| } catch (e) { | ||
| // @ts-ignore | ||
| console.error('Unable to parse response json', e.message) | ||
| throw e instanceof TypeError ? new BodyReadError(e) : e |
2 participants
Why
When a 2xx response body was cut off mid-download,
send()logged it and resolvedundefined, so callers crashed destructuring the result (Sentry MARKETING-APP-K, 3 events). The browser error ("Failed to fetch") has no error code, so the retry config never matched it.What
TypeErrorfromr.json()) now fails with an error that reaches thedefaulthandler, with the original error one.cause.attemptsbudget, only for GET, HEAD and OPTIONS, even ifmethodslists others.Testing
TypeError; invalid JSON, abort and timeout don't. Firefox follows the Fetch spec (4.7, step 18.2.2).Heads up
content-length) used to resolveundefined. It now reaches the error handlers as aSyntaxError. Inside SvelteKit universal loads an empty body still resolvesundefined, as SvelteKit parses it itself.undefined; now it reaches the error handler and isn't retried, so the user may see an error for a write that worked. That's rare (MARKETING-APP-K has 3 events) and safer than repeating the write.