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(node/net): unix domain socket support #2146

Merged
merged 37 commits into from
May 6, 2022

Conversation

cmorten
Copy link
Contributor

@cmorten cmorten commented Apr 23, 2022

Works towards:

Notes:

  • Doesn't add windows support as blocked by Support for Named Pipes in Deno.connect deno#10244.
  • Can't complete the full interface as there is outstanding work needed into Deno core.
  • Improved test coverage for net again after lost from streams rework. Number of tests not passing so more work needed here.
  • Couldn't get some path related tests passing - unsure if an issue with existing net/streams code or missing something in the new work here... but perhaps this is a reasonable start?
  • Files changed looks daunting, but mostly a lot of Node compat tests. Unfortunately formatting updates find the main changes to some files, but hopefully straight-forward reading.

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2022

CLA assistant check
All committers have signed the CLA.

@cmorten cmorten changed the title feat(node/net): unix domain / windows named pipe support feat(node/net): unix domain socket support Apr 24, 2022
@cmorten cmorten marked this pull request as ready for review April 27, 2022 11:57
@cmorten cmorten requested a review from bartlomieju as a code owner April 27, 2022 11:57
@cmorten cmorten requested a review from kt3k as a code owner April 27, 2022 11:57
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM!

Great first pass for ipc conn support in node compat! Also thank you for enabling lot of compat test cases!

@kt3k
Copy link
Member

kt3k commented May 3, 2022

Side note: Confirmed a basic use case working

In node (compat)

const net = require("net");
net.createServer((c) => { 
  c.on("data", (data) => { 
    console.log("on data", data.toString());
    c.end("HTTP/1.1 200 OK\nContent-type: text/html\n\nhello\n");
  });
}).listen("/path/to/my.socket", () => { 
  console.log("server linstening at localhost:3001");
});

Requesting it from deno

const hostname = "example.com"
const conn = await Deno.connect({ path: "/path/to/my.socket", transport: "unix" }); 
const blob = new Blob([`GET / HTTP/1.1
Host: ${hostname}

`]);
const writer = conn.writable.getWriter();
writer.write(new Uint8Array(await blob.arrayBuffer()));
conn.readable.pipeTo(Deno.stdout.writable);

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM again

path: address,
transport: "unix",
};

Deno.connect(connectOptions).then(
DenoUnstable.connect(connectOptions).then(
Copy link
Member

Choose a reason for hiding this comment

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

This is helpful. Thanks!

@kt3k kt3k merged commit 5a07d4d into denoland:main May 6, 2022
@cmorten cmorten deleted the feat/node_net_pipe branch May 6, 2022 08:08
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.

3 participants