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(ext/node): support http2session.socket #24786

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

seanmonstar
Copy link
Contributor

@seanmonstar seanmonstar commented Jul 29, 2024

Continues from #22617

Closes #20903

cc @bartlomieju @satyarohith

@@ -357,7 +428,8 @@ function closeSession(session: Http2Session, code?: number, error?: Error) {

export class ServerHttp2Session extends Http2Session {
constructor() {
super(constants.NGHTTP2_SESSION_SERVER, {});
// TODO(satyarohith): pass socket instead of undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not fully clear on the semantics of Deno <=> Rust ownership, so I don't know if this is easily shared. But if it is, I can just make the change lower in this file:

     this.on(
       "connection",
       (conn: Deno.Conn) => {
         try {
-          const session = new ServerHttp2Session();
+          const session = new ServerHttp2Session(conn);
           this.emit("session", session);
           this.#server = serveHttpOnConnection(
             conn,

Copy link
Member

Choose a reason for hiding this comment

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

This should be okay to do; "Deno.Conn" is a resource backed by a Rust resource. If the Rust resource is moved the worst we get is BadResource error in JS, which seems preferable than not having connection available on the session object at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spoke too eagerly, it seems there isn't a straight forward way to create a net.Socket from a Deno.Conn, or if there is I haven't found it.

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 have to refactor net.Socket to work on top of rust resource that backs Deno.Conn. We can do that in a future iteration.

Copy link
Member

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @seanmonstar!

@satyarohith satyarohith enabled auto-merge (squash) August 14, 2024 11:13
@satyarohith satyarohith merged commit 875ee61 into denoland:main Aug 14, 2024
17 checks passed
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.

Not implemented: Http2Session.socket
3 participants