fix(ext/node): support createConnection option in node:http.request() #25470

satyarohith · 2024-09-05T15:30:13Z

Work in progress. Feature works but breaks existing tests.

Closes #19507

Yoshiya Hinosawa <stibium121@gmail.com>

bartlomieju · 2024-12-04T00:04:02Z

Sorry for a slow turn around. I will review this tomorrow.

bartlomieju · 2024-12-05T13:12:50Z

I will review and merge it after 2.1.3 is released tonight so we have a few days to test it out on canary before putting it in a release build.

bartlomieju · 2024-12-08T22:53:36Z

ext/node/ops/http.rs

+  let res = sender.send_request(request).map_err(Error::from).boxed();
+  let rid = state


Is this on purpose that we don't allow to cancel the request now (ie. the CancelHandle is not created and used here)?

I guess it's not on purpose. @satyarohith What do you think?

I'll try to restore it

It's not on purpose.

👍 to restore it.

Restored the cancel handle in 0e3e450

bartlomieju · 2024-12-08T22:54:09Z

ext/node/ops/http.rs


+  let res = resource.response.await?;


I think we should remap the error here from a generic hyper error to a concrete type we handle in JS

ext/node/polyfills/_http_outgoing.ts

ext/node/polyfills/http.ts

ext/node/polyfills/internal_binding/cares_wrap.ts

bartlomieju · 2024-12-10T13:35:51Z

Possibly closes #26182

0xkalle · 2024-12-10T13:51:38Z

I am also ready to test with #26735 as soon as I should.

bartlomieju

LGTM

blouflashdb · 2024-12-15T11:45:22Z

I this released yet?

0xkalle · 2024-12-15T11:47:23Z

I'd guess as it is on main you can use canary to try it. But as far as I know it's not in a release yet.

bartlomieju · 2024-12-15T11:55:44Z

It's not released yet. please run deno upgrade canary and let us know if it solved the problem for you.

0xkalle · 2024-12-15T13:05:45Z

#26735 fixed on canary.

StreetStrider · 2024-12-15T17:08:16Z

canary + ws works for me now. Thank you for work 👍

Unix domain socket is not working, though, while they're working in the same node's test. Not sure why, it seem to not create socket file.

bartlomieju · 2024-12-16T00:20:08Z

Thanks for the feedback, I'm glad to hear that you got unblocked folks!

Unix domain socket is not working, though, while they're working in the same node's test. Not sure why, it seem to not create socket file.

@StreetStrider could you open a separate issue about this one? Having a specific repro you are using would be so much helpful 🙏

StreetStrider · 2024-12-18T01:28:32Z

@bartlomieju here's the repro
I may be missing something, but I think listening to unix socket is enough for a file to occur. At least that's how it worked in my Node.js+ws test.
deno-sock.tar.gz

blouflashdb · 2024-12-26T17:48:10Z

works now on canary

satyarohith force-pushed the support_create_connection branch from c807b0e to abda518 Compare September 11, 2024 05:33

satyarohith added 12 commits September 24, 2024 11:01

feat: op_node_http_request_with_conn

568cd78

chore(ext/node): remove old ops

c883a72

feat(ext/node): fallback to net.createConnection in client

2c576e8

feat(ext/node): set on("socket") listener for write

a3363a9

feat: use buffer, introduce flushHeaders, flushBody

ae5d72a

flush buffer once socket is ready

821d5a5

introduce more ops to wait for connection ready

29fe176

fix ids

d95db75

wip

95ad0fa

wip

f9c1219

wip

3261473

get get_data.js working

e98525c

satyarohith force-pushed the support_create_connection branch from 89a4de0 to e98525c Compare September 24, 2024 05:32

satyarohith and others added 3 commits September 24, 2024 11:40

wip

65eb113

fmt

be8c90d

clean up debug logs

9c1b39b

kt3k force-pushed the support_create_connection branch from aedab15 to 9c1b39b Compare September 26, 2024 03:45

fixes

2f9cdaa

kt3k force-pushed the support_create_connection branch from bbd15fb to 2f9cdaa Compare September 26, 2024 04:05

update test cases

6301b58

kt3k added the ci-draft Run the CI on draft PRs. label Sep 26, 2024

kt3k added 2 commits September 26, 2024 13:26

fmt

a32ea23

fmt

33c4efa

kt3k changed the title ~~WIP: support createConnection option in node:http.request()~~ fix(ext/node): WIP support createConnection option in node:http.request() Sep 26, 2024

kt3k and others added 5 commits September 26, 2024 13:40

rerun

8aea0f0

fix lint errors

122cefa

wip: abort request when destroy() is invoked

2c60a43

Yoshiya Hinosawa <stibium121@gmail.com>

fmt

7c43c3c

enable [node/http] destroyed requests should not be sent

7a912bf

Merge branch 'main' into support_create_connection

1f817a8

kt3k and others added 4 commits December 4, 2024 11:52

Merge branch 'main' into support_create_connection

502a6d2

reduce compat cases added in denoland#27134

6df2526

reduce further compat test cases for windows

2cbaf00

Merge branch 'main' into support_create_connection

06f0498

Merge branch 'main' into support_create_connection

f55c774

bartlomieju reviewed Dec 8, 2024

View reviewed changes

kt3k added 3 commits December 9, 2024 13:04

refactor: remove unused _listenerSet

46d68b3

fix: do not use requestReady internal event

3ec165c

restore cancel handle for node http request

0e3e450

add undefined check of _req

e85258d

crowlKats and others added 2 commits December 10, 2024 15:18

rework errors

e1bae75

Merge branch 'main' into support_create_connection

97f42ff

bartlomieju approved these changes Dec 12, 2024

View reviewed changes

Merge branch 'main' into support_create_connection

e3069e9

bartlomieju merged commit 960776c into denoland:main Dec 13, 2024
17 checks passed

satyarohith deleted the support_create_connection branch December 13, 2024 05:28

StreetStrider mentioned this pull request Dec 22, 2024

Unix domain socket not working with node:http #27450

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ext/node): support createConnection option in node:http.request() #25470

fix(ext/node): support createConnection option in node:http.request() #25470

satyarohith commented Sep 5, 2024 •

edited

Loading

bartlomieju commented Dec 4, 2024

bartlomieju commented Dec 5, 2024

bartlomieju Dec 8, 2024

kt3k Dec 9, 2024

satyarohith Dec 9, 2024

kt3k Dec 10, 2024

bartlomieju Dec 8, 2024

bartlomieju commented Dec 10, 2024

0xkalle commented Dec 10, 2024

bartlomieju left a comment

blouflashdb commented Dec 15, 2024

0xkalle commented Dec 15, 2024

bartlomieju commented Dec 15, 2024

0xkalle commented Dec 15, 2024

StreetStrider commented Dec 15, 2024

bartlomieju commented Dec 16, 2024

StreetStrider commented Dec 18, 2024 •

edited

Loading

blouflashdb commented Dec 26, 2024

		let res = sender.send_request(request).map_err(Error::from).boxed();
		let rid = state

fix(ext/node): support createConnection option in node:http.request() #25470

fix(ext/node): support createConnection option in node:http.request() #25470

Conversation

satyarohith commented Sep 5, 2024 • edited Loading

bartlomieju commented Dec 4, 2024

bartlomieju commented Dec 5, 2024

bartlomieju Dec 8, 2024

Choose a reason for hiding this comment

kt3k Dec 9, 2024

Choose a reason for hiding this comment

satyarohith Dec 9, 2024

Choose a reason for hiding this comment

kt3k Dec 10, 2024

Choose a reason for hiding this comment

bartlomieju Dec 8, 2024

Choose a reason for hiding this comment

bartlomieju commented Dec 10, 2024

0xkalle commented Dec 10, 2024

bartlomieju left a comment

Choose a reason for hiding this comment

blouflashdb commented Dec 15, 2024

0xkalle commented Dec 15, 2024

bartlomieju commented Dec 15, 2024

0xkalle commented Dec 15, 2024

StreetStrider commented Dec 15, 2024

bartlomieju commented Dec 16, 2024

StreetStrider commented Dec 18, 2024 • edited Loading

blouflashdb commented Dec 26, 2024

satyarohith commented Sep 5, 2024 •

edited

Loading

StreetStrider commented Dec 18, 2024 •

edited

Loading