Skip to content

Commit

Permalink
fix(ext/http): Ensure cancelled requests don't crash Deno.serve (#19154)
Browse files Browse the repository at this point in the history
Fixes for various `Attemped to access invalid request` bugs (#19058,
#15427, #17213).

We did not wait for both a drop event and a completion event before
removing items from the slab table. This ensures that we do so.

In addition, the slab methods are refactored out into `slab.rs` for
maintainability.
  • Loading branch information
mmastrac authored May 16, 2023
1 parent 9ba2c4c commit a22388b
Show file tree
Hide file tree
Showing 7 changed files with 416 additions and 313 deletions.
33 changes: 32 additions & 1 deletion cli/tests/unit/serve_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
deferred,
fail,
} from "./test_util.ts";
import { consoleSize } from "../../../runtime/js/40_tty.js";

const {
upgradeHttpRaw,
Expand Down Expand Up @@ -665,6 +664,38 @@ Deno.test({ permissions: { net: true } }, async function httpServerClose() {
await server;
});

// https://github.com/denoland/deno/issues/15427
Deno.test({ permissions: { net: true } }, async function httpServerCloseGet() {
const ac = new AbortController();
const listeningPromise = deferred();
const requestPromise = deferred();
const responsePromise = deferred();
const server = Deno.serve({
handler: async () => {
requestPromise.resolve();
await new Promise((r) => setTimeout(r, 500));
responsePromise.resolve();
return new Response("ok");
},
port: 4501,
signal: ac.signal,
onListen: onListen(listeningPromise),
onError: createOnErrorCb(ac),
});
await listeningPromise;
const conn = await Deno.connect({ port: 4501 });
const encoder = new TextEncoder();
const body =
`GET / HTTP/1.1\r\nHost: example.domain\r\nConnection: close\r\n\r\n`;
const writeResult = await conn.write(encoder.encode(body));
assertEquals(body.length, writeResult);
await requestPromise;
conn.close();
await responsePromise;
ac.abort();
await server;
});

// FIXME:
Deno.test(
{ permissions: { net: true } },
Expand Down
2 changes: 2 additions & 0 deletions ext/http/00_serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,8 @@ function mapToCallback(responseBodies, context, signal, callback, onError) {

// Did everything shut down while we were waiting?
if (context.closed) {
// We're shutting down, so this status shouldn't make it back to the client but "Service Unavailable" seems appropriate
op_http_set_promise_complete(req, 503);
innerRequest?.close();
return;
}
Expand Down
1 change: 1 addition & 0 deletions ext/http/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ description = "HTTP server implementation for Deno"

[features]
"__zombie_http_tracking" = []
"__http_tracing" = []

[lib]
path = "lib.rs"
Expand Down
Loading

0 comments on commit a22388b

Please sign in to comment.