Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Bind to both IPv4 and IPv6 when localhost is specified (#231). #870

Merged
merged 1 commit into from
May 26, 2016

Conversation

cesarblum
Copy link
Contributor


if (parsedAddress.Host.Equals("localhost", StringComparison.OrdinalIgnoreCase))
{
var exceptions = new List<Exception>();
Copy link
Member

Choose a reason for hiding this comment

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

Creating and adding to a list just to get a count? Why not just use a counter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was different before and I didn't revert this. I was throwing an AggregateException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that might be better?

@halter73
Copy link
Member

Travis is producing a bunch of Error -99 EADDRNOTAVAIL address not available errors in travis tests on both OS X and Linux: https://travis-ci.org/aspnet/KestrelHttpServer/jobs/132455305


// Make sure we bind to the same port on port 0.
var port = ipv4Address.Port;
var ipv6Address = parsedAddress.WithHost("[::1]").WithPort(port);
Copy link
Member

Choose a reason for hiding this comment

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

Much more clear now 👍

@cesarblum
Copy link
Contributor Author

No more filters.

@halter73
Copy link
Member

:shipit:

@cesarblum cesarblum force-pushed the cesarbs/bind-localhost-to-ipv4-and-ipv6 branch from dbbda16 to 373d0ac Compare May 25, 2016 21:37
@cesarblum
Copy link
Contributor Author

Updated to disallow binding to localhost:0. cc @davidfowl

}

// Make sure we bind to the same port on port 0.
var port = ipv4Address.Port;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this logic since we no longer support localhost:0.

@cesarblum cesarblum force-pushed the cesarbs/bind-localhost-to-ipv4-and-ipv6 branch 2 times, most recently from 85d9a33 to 6da59d5 Compare May 25, 2016 21:53
@cesarblum
Copy link
Contributor Author

🔔

@@ -81,7 +81,6 @@ public void NoDelay(bool enable)
/// </summary>
public static IPEndPoint CreateIPEndpoint(ServerAddress address)
{
// TODO: IPv6 support
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave this comment in for now. I can remove it when I fix #766.

@mikeharder
Copy link
Contributor

You can :shipit: after addressing my comments. Or, you could wait until I figure out the design for #766, since it might impact this change as well.

@cesarblum cesarblum force-pushed the cesarbs/bind-localhost-to-ipv4-and-ipv6 branch from e033a33 to a3d0bd0 Compare May 26, 2016 04:23
@cesarblum cesarblum merged commit a3d0bd0 into dev May 26, 2016
@cesarblum cesarblum deleted the cesarbs/bind-localhost-to-ipv4-and-ipv6 branch May 26, 2016 04:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants