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: Implement QUIC #21942

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Implement QUIC #21942

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jan 15, 2024

Implements a QUIC interface, loosely based on the WebTransport API (a future change could add the WebTransport API, built on top of this one).

quinn is used for the underlying QUIC implementation, for a few reasons:

  • A cloneable "handle" api which fits quite nicely into deno resources.
  • Good collaboration with the rust ecosystem, especially rustls.
  • I like it.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

nice API 👍

Cargo.lock Outdated Show resolved Hide resolved
cli/tests/unit/quic_test.ts Outdated Show resolved Hide resolved
@devsnek devsnek changed the title [wip] quic Implement QUIC Jan 15, 2024
@devsnek devsnek marked this pull request as ready for review January 15, 2024 06:53
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I agree with @littledivy's comments re: ring.

I think we should consider merging this into ext/net rather than creating a new module, however.

ext/quic/01_quic.js Outdated Show resolved Hide resolved
@devsnek devsnek changed the title Implement QUIC feat: Implement QUIC Jan 15, 2024
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.

@devsnek this is fantastic work, I'm impressed how few lines of code you needed to implement that 💪

ext/quic/01_quic.js Outdated Show resolved Hide resolved
ext/quic/01_quic.js Outdated Show resolved Hide resolved
ext/quic/lib.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

@devsnek we'll get more reviews in this week and aim to land it for v1.41.

ext/quic/01_quic.js Outdated Show resolved Hide resolved
@mmastrac mmastrac added this to the 1.41 milestone Feb 13, 2024
@hironichu
Copy link
Contributor

hironichu commented Feb 19, 2024

How hard or how long would it take to implement Webtransport with this PR ?

My suggestion is : why not adding it at the same time before merging in 1.41 since WT is 76% available on the browsers.

Adding a client seems "easy" enough, and a server which is upgrading the QUIC transport connection to a server (somewhat like Websocket works at the moment)

@bartlomieju bartlomieju removed this from the 1.41 milestone Mar 6, 2024
@lino-levan
Copy link
Contributor

How hard or how long would it take to implement WebTransport with this PR ?

Probably should be done in a separate PR imo. I'm really excited about this! Is there any consideration for this in deno 1.43 @bartlomieju?

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Very nicely done. Most of my suggestions are little nits. However, same as @mmastrac, I question whether this should be done as an extra module rather than within ext/net.

Either way, can you please resolve the merge conflicts?

ext/quic/lib.deno_quic.d.ts Outdated Show resolved Hide resolved
ext/quic/lib.deno_quic.d.ts Outdated Show resolved Hide resolved
ext/quic/lib.deno_quic.d.ts Outdated Show resolved Hide resolved
ext/quic/lib.deno_quic.d.ts Outdated Show resolved Hide resolved
ext/quic/lib.deno_quic.d.ts Outdated Show resolved Hide resolved
ext/quic/01_quic.js Outdated Show resolved Hide resolved
ext/quic/01_quic.js Outdated Show resolved Hide resolved
ext/quic/01_quic.js Outdated Show resolved Hide resolved
ext/quic/01_quic.js Outdated Show resolved Hide resolved
Comment on lines 361 to 363
connectQuic: quic.connectQuic,
listenQuic: quic.listenQuic,
QuicBidirectionalStream: quic.QuicBidirectionalStream,
QuicConn: quic.QuicConn,
QuicListener: quic.QuicListener,
QuicReceiveStream: quic.QuicReceiveStream,
QuicSendStream: quic.QuicSendStream,
};

Copy link
Contributor

Choose a reason for hiding this comment

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

We can either include these classes in the Deno.* namespace or make the constructors of these classes illegal. The former adds to the API surface area, but the latter enables such objects to be identified with the use of instanceof. Which should we do? @bartlomieju

@hironichu
Copy link
Contributor

Very nicely done. Most of my suggestions are little nits. However, same as @mmastrac, I question whether this should be done as an extra module rather than within ext/net.

Either way, can you please resolve the merge conflicts?

I also think we should merge it into ext/net, and if we want to make protocol that depends on QUIC then we can make module ( i.e WebTransport and Media over Quic (MoQ)..

@devsnek devsnek force-pushed the quic branch 2 times, most recently from 4fe503b to e2156ca Compare July 10, 2024 01:37
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Mostly documentation nits.

maxIdleTimeout?: number;
/** Maximum number of incoming bidirectional streams that may be open
* concurrently.
* Default: 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Default: 100
* @default {100}

maxConcurrentBidirectionalStreams?: number;
/** Maximum number of incoming unidirectional streams that may be open
* concurrently.
* Default: 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Default: 100
* @default {100}

export interface ListenQuicOptions extends QuicTransportOptions {
/** The port to connect to. */
port: number;
/** A literal IP address or host name that can be resolved to an IP address. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** A literal IP address or host name that can be resolved to an IP address. */
/** A literal IP address or host name that can be resolved to an IP address.
*
* @default {"0.0.0.0"}
*/

Should we also add the same note that Deno.ServeOptions.hostname has?

Comment on lines +655 to +657
accept(): Promise<QuicConn>;
refuse(): Promise<void>;
ignore(): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these have descriptions?

* which the value has been set. */
sendOrder?: number;
/** Wait until there is sufficient flow credit to create the stream.
* Defaults to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Defaults to false.
* @default {false}

export interface QuicSendStreamOptions {
/** Indicates the send priority of this stream relative to other streams for
* which the value has been set. */
sendOrder?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the default behavior of this property?

@lkwr
Copy link

lkwr commented Sep 25, 2024

Any updates on this? @devsnek @bartlomieju

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.

9 participants