Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(unstable): Add Deno.upgradeHttp API #13618

Merged
merged 21 commits into from
Mar 16, 2022

Conversation

piscisaureus
Copy link
Member

@piscisaureus piscisaureus commented Feb 7, 2022

fixes #12718

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not very happy with this API. It isn't going to work well in std/http.

@crowlKats
Copy link
Member

@lucacasonato its very WIP still

@piscisaureus
Copy link
Member Author

piscisaureus commented Feb 9, 2022

My preferred API:

const reqEvent = await httpConn.nextRequest();
if (!e) return;
const [conn, readBuf]: [Conn, Uint8Array] = await reqEvent.upgradeWith(new Response(null));

It isn't going to work well in std/http

I'm sure there's away to add support for this to std/http, although I haven't thought of how.

@crowlKats
Copy link
Member

crowlKats commented Feb 9, 2022

Ideally something like this:
inteface:

  export interface RawUpgrade {
    response: Response;
    conn: Promise<Deno.Conn | Deno.TlsConn>;
  }

  export function upgradeRaw(): RawUpgrade;

example:

async function server() {
  let tcp_server = Deno.listen({ host: "127.0.0.1", port: 8080 });
  let tcp_conn = await tcp_server.accept();
  let http_conn = Deno.serveHttp(tcp_conn);
  let http_event = await http_conn.nextRequest();
  const { connPromise, response } = Deno.upgradeRaw({ status: 101 });
  http_event.respondWith(response);
  let [conn, firstPacket] = await connPromise; // its Deno.Conn or Deno.TlsConn
}

@ry
Copy link
Member

ry commented Feb 15, 2022

We've discussed this and came up with this API

import { serve } from "https://deno.land/std/http/server.ts";

serve((req) => {

  let p = Deno.upgradeHttp(req);

  (async () {
    let [conn, firstPacket] = await p;
    // ...
  })();

  // this would hang forever. don't do the following:
  // await p; 

  return new Response({ status: 101 });
});

@piscisaureus piscisaureus requested a review from kitsonk as a code owner March 2, 2022 15:44
@biluohc
Copy link
Contributor

biluohc commented Mar 5, 2022

I think the firstPacket is very ugly, may be better to use hyper's Upgrade directly?

In that case, we don't need to deal with the difference between tcp/uds connections and it's tls wrapped, and we don't need the first-packet

@crowlKats
Copy link
Member

We cant return return Upgrade directly; we'd need to create some new class then on JS side for that. Also being able to differentiate if it is tcp or tls means different functionality can be used (as in, tcp has some methods that tls doesnt). API wise this is just the best.

@biluohc
Copy link
Contributor

biluohc commented Mar 5, 2022

Ok, the original structure has more control, come on

@ry ry changed the title WIP: http connection hijacking http connection hijacking Mar 14, 2022
@ry
Copy link
Member

ry commented Mar 14, 2022

  • needs test.js made into real unit test
  • needs to be unstable API

@ry ry assigned bartlomieju and unassigned crowlKats Mar 14, 2022
@bartlomieju bartlomieju changed the title http connection hijacking feat(unstable): Add Deno.upgradeHttp API Mar 15, 2022
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartlomieju bartlomieju merged commit c5270ab into denoland:main Mar 16, 2022
Comment on lines +137 to +140
Err(_) => Err(custom_error(
"Http",
"encountered unsupported transport while upgrading",
)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we also support Unix transport?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP hijacking (for Node WS/CONNECT, etc...)
6 participants