feat(ext/http): add support for unix domain sockets

[ylxdzsw]

This adds support for unix domain sockets in ext/http (#13626). The primary change is replace std::net::SocketAddr with a custom enum so we can store the address of a unix domain socket. I have to make this type pub in order to refer to it in runtime/ops/http.rs .

[CLAassistant]

All committers have signed the CLA.

[bartlomieju]

Thanks, this looks quite good, but we definitely need some tests to be able to land it.

[ylxdzsw]

@bartlomieju Hi, I'm willing to add some tests, but I don't know where to put them. I didn't find existing tests for the ext/http crate. Should I create a tests folder for the ext/http crate and test in Rust, or somehow test it in Typescript?

[bartlomieju]

Sorry I forgot to mention. Tests should be added to cli/tests/unit/http_test.ts

[ylxdzsw]

@bartlomieju Thanks. I have added a test for it. However I'm not very sure what the url should look like. I have chosen http+unix://percent_encoding/ (used by httpie) as the other popular choice http://unix:[path]:/ (used by nginx) is not parsed properly by new URL(). I'm open to change it though.

[bartlomieju]

@bartlomieju Thanks. I have added a test for it. However I'm not very sure what the url should look like. I have chosen http+unix://percent_encoding/ (used by httpie) as the other popular choice http://unix:[path]:/ (used by nginx) is not parsed properly by new URL(). I'm open to change it though.

I'm not sure I understand. Where is this URL being used?

[ylxdzsw]

The URL is used to construct the Request object which finally expose to the user. Example:

const httpConn = Deno.serveHttp(conn)
for await (const { request, respondWith } of httpConn) {
    console.log(request.url)
}

Currently it is constructed using the Host header and fallbacks to the socket address. However, both are not avaliable in Unix sockets.

[bartlomieju]

@lucacasonato what do you think about the URL?

[lucacasonato]

The URL pattern seems fine. By extension of Deno.listen(transport: unix) being unstable, this would be unstable too. As such I think we can merge it for 1.19.

Just have a minor comment on the test.

[bartlomieju]

Declaration files should be updated for Deno.serveHttp to hint that unix sockets can be used, and as Luca pointed out it should be marked as unstable

[ylxdzsw]

Hi @bartlomieju , I have added the unstable check. About the declaration, Deno.serveHttp only takes Conn as argument and one can already pass a Conn created by Deno.listen({ transport: "unix"}) to it. It just throws Bad resource ID error currently as I have reported at #13626. I don't think we need to modify (and I don't know how if needed) the declaration file for this addtion.

[ylxdzsw]

Hi, any idea about the CI failling? It passes locally and the last edit is only adding the unstable check. I can't interpret the CI output.

[bartlomieju]

Hi, any idea about the CI failling? It passes locally and the last edit is only adding the unstable check. I can't interpret the CI output.

I landed a PR that broke main. Please merge main branch again and it should go green.

[lucacasonato]

LGTM. Great job @ylxdzsw! 🎉