Skip to content

Commit 8402ecf

Browse files
Jake ChampionJakeChampion
authored andcommitted
fix: When streaming a response to the host, do not close the response body if an error occurs
Previously, if an error occurred during streaming, we would close the body, which is the host inteprets as a successful response and so the response body would contain `0\r\n\r\n` at the end which indicates the successful termination of a chunk-encoded response however, this was not meant to be a successful response as an error occured. This commit changes the behavior to not close the response body when an error occurs, which in turn means `0\r\n\r\n` is not added to the end of the response and the downstream client will know that the response was not successfully terminated. We can't yet confirm this behaviour with a test as neither viceroy nor c@e have the latest version of the ABI. I've confirmed this is working with a manual test using a local version of Viceroy which has the new ABI. This is the program I tested: ```js /* eslint-env serviceworker */ /* global ReadableStream */ /// <reference path="../../../../../types/index.d.ts" /> import { env } from 'fastly:env'; import { fail } from "../../../assertions.js"; addEventListener("fetch", event => { event.respondWith(app(event)) }) /** * @param {FetchEvent} event * @returns {Response} */ async function app(event) { try { const path = (new URL(event.request.url)).pathname; console.log(`path: ${path}`) console.log(`FASTLY_SERVICE_VERSION: ${env('FASTLY_SERVICE_VERSION')}`) if (routes.has(path)) { const routeHandler = routes.get(path); return await routeHandler() } return fail(`${path} endpoint does not exist`) } catch (error) { return fail(`The routeHandler threw an error: ${error.message}` + '\n' + error.stack) } } const routes = new Map(); routes.set('/', () => { routes.delete('/'); let test_routes = Array.from(routes.keys()) return new Response(JSON.stringify(test_routes), { 'headers': { 'content-type': 'application/json' } }); }); routes.set('/streaming/controller-error-method-called', async () => { const encoder = new TextEncoder() let flag = false; const stream = new ReadableStream({ async start(controller) { controller.enqueue(encoder.encode('This ')); }, async pull(controller) { if (flag) { controller.error(new Error('powpowpowpow')); } else { flag = true; } } }); return new Response(stream); }); routes.set('/streaming/runtime-error-thrown', async () => { const encoder = new TextEncoder() let flag = false; const stream = new ReadableStream({ async start(controller) { controller.enqueue(encoder.encode('This ')); }, async pull() { if (flag) { throw new Error('powpowpowpow'); } else { flag = true; } } }); return new Response(stream); }); ``` And this is the test request and responses: ``` ❯ curl http://127.0.0.1:7878/streaming/runtime-error-thrown --no-buffer This curl: (18) transfer closed with outstanding read data remaining ❯ curl http://127.0.0.1:7878/streaming/controller-error-method-called --no-buffer This curl: (18) transfer closed with outstanding read data remaining ```
1 parent 12001ab commit 8402ecf

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

c-dependencies/js-compute-runtime/js-compute-builtins.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1213,7 +1213,7 @@ bool body_reader_catch_handler(JSContext *cx, HandleObject body_owner, HandleVal
12131213
if (Response::is_instance(body_owner)) {
12141214
FetchEvent::set_state(FetchEvent::instance(), FetchEvent::State::responseDone);
12151215
}
1216-
return HANDLE_RESULT(cx, xqd_body_close(RequestOrResponse::body_handle(body_owner)));
1216+
return true;
12171217
}
12181218

12191219
/**

0 commit comments

Comments
 (0)