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

fix(ext/node): add autoSelectFamily option to net.createConnection #26661

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Oct 31, 2024

closes #26641

  • compat test (tests/node_compat/test/parallel/test-net-autoselectfamily.js) is too slow in mac (like 1m30s)
  • compat test (tests/node_compat/test/parallel/test-net-autoselectfamily.js) doesn't pass on windows
  • http2.connect("https://example.com") doesn't work

@kt3k kt3k added the ci-draft Run the CI on draft PRs. label Oct 31, 2024
@kt3k kt3k force-pushed the node-net-autoselectfamily branch from 04fc611 to d3d005b Compare October 31, 2024 11:50
@kt3k
Copy link
Member Author

kt3k commented Nov 8, 2024

The second createDnsServer call seems taking 1m30s, and that seems caused because of some ops remaining even after dnsServer.close() call (Something looks wrong with dgram.Socket. I'll look into them further)

@kt3k kt3k marked this pull request as ready for review November 11, 2024 15:29
// Create a DNS server which replies with a AAAA and a A record for the same host
const socket = dgram.createSocket('udp4');

// Note(kt3k): We use common.mustCallAtLeast instead of common.mustCall
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should be a TODO

once both the TODOs are resolved we can remove this test from ignore in node_compat/config.jsonc

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks

once both the TODOs are resolved we can remove this test from ignore in node_compat/config.jsonc

That's correct

@@ -393,6 +408,107 @@ function _afterConnect(
}
}

function _createConnectionError(req, status) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we can just remove the underscore prefix for all the functions so they're easy to search in nodejs src too? or is something preventing us from doing that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can just remove the underscore prefix for all the functions so they're easy to search in nodejs src too?

Sounds reasonable to me, but I would do that in another PR as there's so many of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Nice work @kt3k!

LGTM

@kt3k kt3k merged commit c3c2b37 into denoland:main Nov 12, 2024
17 checks passed
@kt3k kt3k deleted the node-net-autoselectfamily branch November 12, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-draft Run the CI on draft PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ext/node: implement autoSelectFamily option in net.connect
2 participants