From f3bde1d53b4710fb526286e27af29a55f5da18c7 Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Wed, 24 Aug 2022 14:10:57 +0200 Subject: [PATCH] feat(ext/flash): split upgradeHttp into two APIs (#15557) This commit splits `Deno.upgradeHttp` into two different APIs, because the same API is currently overloaded with two different functions. Flash requests upgrade immediately, with no need to return a `Response` object. Instead you have to manually write the response to the socket. Hyper requests only upgrade once a `Response` object has been sent. These two behaviours are now split into `Deno.upgradeHttp` and `Deno.upgradeHttpRaw`. The latter is flash only. The former only supports hyper requests at the moment, but can be updated to support flash in the future. Additionally this removes `void | Promise` as valid return types for the handler function. If one wants to use `Deno.upgradeHttpRaw`, they will have to type cast the handler signature - the signature is meant for the 99.99%, and should not be complicated for the 0.01% that use `Deno.upgradeHttpRaw()`. --- cli/bench/testdata/deno_upgrade_http.js | 4 +-- cli/dts/lib.deno.unstable.d.ts | 42 ++++++++++++++++++++----- cli/tests/unit/flash_test.ts | 6 ++-- ext/flash/01_http.js | 26 +++++++++++++-- ext/http/01_http.js | 14 ++------- runtime/js/90_deno_ns.js | 3 +- 6 files changed, 69 insertions(+), 26 deletions(-) diff --git a/cli/bench/testdata/deno_upgrade_http.js b/cli/bench/testdata/deno_upgrade_http.js index 7274459c96..db14f85895 100644 --- a/cli/bench/testdata/deno_upgrade_http.js +++ b/cli/bench/testdata/deno_upgrade_http.js @@ -1,8 +1,8 @@ -const { serve, upgradeHttp } = Deno; +const { serve, upgradeHttpRaw } = Deno; const u8 = Deno.core.encode("HTTP/1.1 101 Switching Protocols\r\n\r\n"); async function handler(req) { - const [conn, _firstPacket] = upgradeHttp(req); + const [conn, _firstPacket] = upgradeHttpRaw(req); await conn.write(u8); await conn.close(); } diff --git a/cli/dts/lib.deno.unstable.d.ts b/cli/dts/lib.deno.unstable.d.ts index a1817deb0b..2f6a197bc5 100644 --- a/cli/dts/lib.deno.unstable.d.ts +++ b/cli/dts/lib.deno.unstable.d.ts @@ -1344,25 +1344,51 @@ declare namespace Deno { options: ServeInit & (ServeOptions | ServeTlsOptions), ): Promise; - /** **UNSTABLE**: new API, yet to be vetter. + /** **UNSTABLE**: new API, yet to be vetted. * - * Allows to "hijack" a connection that the request is associated with. - * Can be used to implement protocols that build on top of HTTP (eg. + * Allows "hijacking" the connection that the request is associated with. + * This can be used to implement protocols that build on top of HTTP (eg. * WebSockets). * - * The return type depends if `request` is coming from `Deno.serve()` API - * or `Deno.serveHttp()`; for former it returns the connection and first - * packet, for latter it returns a promise. - * * The returned promise returns underlying connection and first packet * received. The promise shouldn't be awaited before responding to the * `request`, otherwise event loop might deadlock. * + * ```ts + * function handler(req: Request): Response { + * Deno.upgradeHttp(req).then(([conn, firstPacket]) => { + * // ... + * }); + * return new Response(null, { status: 101 }); + * } + * ``` + * + * This method can only be called on requests originating the `Deno.serveHttp` + * server. + * * @category HTTP Server */ export function upgradeHttp( request: Request, - ): [Deno.Conn, Uint8Array] | Promise<[Deno.Conn, Uint8Array]>; + ): Promise<[Deno.Conn, Uint8Array]>; + + /** **UNSTABLE**: new API, yet to be vetted. + * + * Allows "hijacking" the connection that the request is associated with. + * This can be used to implement protocols that build on top of HTTP (eg. + * WebSockets). + + * Unlike `Deno.upgradeHttp` this function does not require that you respond + * to the request with a `Response` object. Instead this function returns + * the underlying connection and first packet received immediately, and then + * the caller is responsible for writing the response to the connection. + * + * This method can only be called on requests originating the `Deno.serve` + * server. + * + * @category HTTP Server + */ + export function upgradeHttpRaw(request: Request): [Deno.Conn, Uint8Array]; /** @category Sub Process */ export interface SpawnOptions { diff --git a/cli/tests/unit/flash_test.ts b/cli/tests/unit/flash_test.ts index f594842919..c718c1b2ec 100644 --- a/cli/tests/unit/flash_test.ts +++ b/cli/tests/unit/flash_test.ts @@ -1002,14 +1002,14 @@ Deno.test( }, ); -Deno.test("upgradeHttp tcp", async () => { +Deno.test("upgradeHttpRaw tcp", async () => { const promise = deferred(); const listeningPromise = deferred(); const promise2 = deferred(); const ac = new AbortController(); const signal = ac.signal; const handler = async (req: Request) => { - const [conn, _] = await Deno.upgradeHttp(req); + const [conn, _] = Deno.upgradeHttpRaw(req); await conn.write( new TextEncoder().encode("HTTP/1.1 101 Switching Protocols\r\n\r\n"), @@ -1028,6 +1028,8 @@ Deno.test("upgradeHttp tcp", async () => { conn.close(); }; const server = Deno.serve({ + // NOTE: `as any` is used to bypass type checking for the return value + // of the handler. handler: handler as any, port: 4501, signal, diff --git a/ext/flash/01_http.js b/ext/flash/01_http.js index a9ae9d85ae..962b22729c 100644 --- a/ext/flash/01_http.js +++ b/ext/flash/01_http.js @@ -3,7 +3,9 @@ ((window) => { const { BlobPrototype } = window.__bootstrap.file; - const { fromFlashRequest, toInnerResponse } = window.__bootstrap.fetch; + const { TcpConn } = window.__bootstrap.net; + const { fromFlashRequest, toInnerResponse, _flash } = + window.__bootstrap.fetch; const core = window.Deno.core; const { ReadableStream, @@ -18,7 +20,6 @@ _readyState, _eventLoop, _protocol, - _server, _idleTimeoutDuration, _idleTimeoutTimeout, _serverHandleIdleTimeout, @@ -597,7 +598,28 @@ }); } + function upgradeHttpRaw(req) { + if (!req[_flash]) { + throw new TypeError( + "Non-flash requests can not be upgraded with `upgradeHttpRaw`. Use `upgradeHttp` instead.", + ); + } + + // NOTE(bartlomieju): + // Access these fields so they are cached on `req` object, otherwise + // they wouldn't be available after the connection gets upgraded. + req.url; + req.method; + req.headers; + + const { serverId, streamRid } = req[_flash]; + const connRid = core.ops.op_flash_upgrade_http(streamRid, serverId); + // TODO(@littledivy): return already read first packet too. + return [new TcpConn(connRid), new Uint8Array()]; + } + window.__bootstrap.flash = { serve, + upgradeHttpRaw, }; })(this); diff --git a/ext/http/01_http.js b/ext/http/01_http.js index 6df26d09f2..63023a2964 100644 --- a/ext/http/01_http.js +++ b/ext/http/01_http.js @@ -477,17 +477,9 @@ function upgradeHttp(req) { if (req[_flash]) { - // NOTE(bartlomieju): - // Access these fields so they are cached on `req` object, otherwise - // they wouldn't be available after the connection gets upgraded. - req.url; - req.method; - req.headers; - - const { serverId, streamRid } = req[_flash]; - const connRid = core.ops.op_flash_upgrade_http(streamRid, serverId); - // TODO(@littledivy): return already read first packet too. - return [new TcpConn(connRid), new Uint8Array()]; + throw new TypeError( + "Flash requests can not be upgraded with `upgradeHttp`. Use `upgradeHttpRaw` instead.", + ); } req[_deferred] = new Deferred(); diff --git a/runtime/js/90_deno_ns.js b/runtime/js/90_deno_ns.js index dfca9d5bc0..200bf73c2d 100644 --- a/runtime/js/90_deno_ns.js +++ b/runtime/js/90_deno_ns.js @@ -109,7 +109,6 @@ serveHttp: __bootstrap.http.serveHttp, resolveDns: __bootstrap.net.resolveDns, upgradeWebSocket: __bootstrap.http.upgradeWebSocket, - upgradeHttp: __bootstrap.http.upgradeHttp, kill: __bootstrap.process.kill, addSignalListener: __bootstrap.signals.addSignalListener, removeSignalListener: __bootstrap.signals.removeSignalListener, @@ -154,5 +153,7 @@ spawn: __bootstrap.spawn.spawn, spawnSync: __bootstrap.spawn.spawnSync, serve: __bootstrap.flash.serve, + upgradeHttp: __bootstrap.http.upgradeHttp, + upgradeHttpRaw: __bootstrap.flash.upgradeHttpRaw, }; })(this);