Skip to content

Support unix domain sockets in fetch() - #9097

Merged
Jarred-Sumner merged 11 commits into
mainfrom
jarred/unix-domain
Mar 6, 2024
Merged

Jarred-Sumner merged 11 commits into
mainfrom
jarred/unix-domain

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Feb 25, 2024 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

This adds an option "unix" to fetch that lets you provide a path to a unix domain socket. I'm not sure about this API in particular, but this needs to exist somehow

How did you verify your code works?

Tests in fetch.unix.test.ts

@github-actions

github-actions Bot commented Feb 25, 2024 •

Copy link
Copy Markdown
Contributor

❌ @Jarred-Sumner 1 files with test failures on bun-darwin-aarch64:

View test output

#6143d63c685fb80d3c93ed915cccb8dd2846a7a6

@github-actions

github-actions Bot commented Feb 25, 2024 •

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Feb 25, 2024 •

Copy link
Copy Markdown
Contributor

@argosphil

Copy link
Copy Markdown
Contributor

Can we keep the naming consistent between Bun.serve (which uses unix) and fetch?

@gvilums
gvilums marked this pull request as ready for review March 5, 2024 10:22
@gvilums

gvilums commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

Can we keep the naming consistent between Bun.serve (which uses unix) and fetch?

changed to use unix in fetch

@github-actions

github-actions Bot commented Mar 5, 2024 •

Copy link
Copy Markdown
Contributor

❌ @Jarred-Sumner 1 files with test failures on bun-darwin-x64:

View test output

#6143d63c685fb80d3c93ed915cccb8dd2846a7a6

@github-actions

github-actions Bot commented Mar 5, 2024 •

Copy link
Copy Markdown
Contributor

❌🪟 @Jarred-Sumner, there are 12 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\run\as-node.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts

Full Test Output

@gvilums
gvilums requested a review from cirospaciari March 5, 2024 12:10

@cirospaciari cirospaciari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

some comments

Comment thread src/bun.js/webcore/response.zig Outdated
Comment thread src/bun.js/webcore/response.zig
Comment thread src/http.zig
return try this.context(is_ssl).connectSocket(client, client.unix_socket_path);
}

if (client.http_proxy) |url| {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should we error on Bun__fetch if we try to use http_proxy + unix socket?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah we probably should

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

added

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we also need a test for this case

Comment thread test/js/web/fetch/fetch.unix.test.ts
@gvilums
gvilums requested a review from cirospaciari March 6, 2024 13:50

@cirospaciari cirospaciari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one comment and test/js/node/stream/node-stream.test.js (SIGTERM) on bun-darwin-aarch64

Comment thread test/js/web/fetch/fetch.unix.test.ts Outdated
Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator Author

The node-stream test is a flaky one, I think unrelated to these changes

@Jarred-Sumner
Jarred-Sumner merged commit a669262 into main Mar 6, 2024
@Jarred-Sumner
Jarred-Sumner deleted the jarred/unix-domain branch March 6, 2024 22:12
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator Author

Please add a test for this:

should we error on Bun__fetch if we try to use http_proxy + unix socket?

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.

4 participants