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

happy eyeballs #11206

Merged
merged 16 commits into from
May 21, 2024
Merged

happy eyeballs #11206

merged 16 commits into from
May 21, 2024

Conversation

gvilums
Copy link
Contributor

@gvilums gvilums commented May 20, 2024

What does this PR do?

When connecting a socket we initiate simultaneous connections to all addresses returned by getaddrinfo, and keep the first one.

How did you verify your code works?

Existing tests.

@Jarred-Sumner
Copy link
Collaborator

Existing tests.

I don't think we can get away with not adding new tests for this.

Can you add tests that check it closes connections when it should? Can you add tests that check on_open isn't called for multiple sockets that were attempted? I think there are more cases to handle than that

*is_connecting = 1;
return (struct us_connecting_socket_t *) us_socket_context_connect_resolved_dns(context, ptr, options, socket_ext_size);

// with happy eyeballs we can't use the fast path since we need to initiate multiple connections
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not quite accurate

we only should go through the happy eyeballs case when there are multiple results from addrinfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, I updated the code to use the fast path when there is a cached addrinfo result that has only one entry

Copy link
Contributor

github-actions bot commented May 20, 2024

@gvilums, your commit has failing tests :(

💪 4 failing tests Darwin AARCH64

💻 3 failing tests Darwin x64 baseline

💻 4 failing tests Darwin x64

🐧💪 1 failing tests Linux AARCH64

🐧🖥 1 failing tests Linux x64 baseline

🐧🖥 2 failing tests Linux x64

🪟💻 13 failing tests Windows x64 baseline

🪟💻 12 failing tests Windows x64

View logs

@gvilums
Copy link
Contributor Author

gvilums commented May 21, 2024

One of the test failures ("request body and signal life cycle" in serve.test.ts) is not caused by this PR, it can be reproduced on stable bun as shown in #11208

@Jarred-Sumner
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner merged commit 4df387d into main May 21, 2024
45 of 53 checks passed
@Jarred-Sumner Jarred-Sumner deleted the georgijs/happy-eyeballs branch May 21, 2024 05:12

break :brk allocator.dupeZ(u8, raw_host) catch bun.outOfMemory();
};
// remove brackets from IPv6 addresses, as getaddrinfo doesn't understand them
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think there are probably like 10 more cases here to handle btw

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.

2 participants