search

vapor-community / sockets

🔌 Non-blocking TCP socket layer, with event-driven server and client.
MIT License
575 stars 54 forks source link

resolve all addresses #137

Closed tanner0101 closed 6 years ago

tanner0101 commented 7 years ago

Fixes https://github.com/vapor/vapor/issues/1086

tanner0101 commented 7 years ago

cc/ @ckd, @vzsg, @TG908 can you confirm if this fixes the localhost resolve issue? If so, I can merge and tag as 2.2.0.

vzsg commented 7 years ago

I'll give it a try soon.

vzsg commented 7 years ago

I manually checked out the all-resolved-addresses branch in my .build folder to test this. I see that two addresses are returned for localhost; however, neither could connect successfully...

So I tried using Node instead of Python's SimpleHTTPServer to see if it makes a difference, and it worked! Unfortunately, it also worked without this PR applied, so @TG908's observation about Python binding to IPv4 only seems to be on the spot.

tl;dr: This is definitely an improvement, although doesn't fix the original issue, as the engine client still doesn't fall back to IPv4, unlike the stronger FoundationClient.

tgymnich commented 7 years ago

I think the issue is with the socket descriptor. The same socket descriptor is used for both IPv4 and IPv6.

var conf = Config.TCP(addressFamily: address.addressFamily)
let resolved = try address.resolve(with: &conf)
let descriptor = try Descriptor(conf)

The descriptor is only set once for the first address.

I'd suggest saving the config for each ResolvedAddress like this[ResolvedInternetAddress: Config] and generate new socket descriptors when needed.

Another way would be to save all the descriptors in an array just like the addresses.

I dont have the time now to work out a full PR. Will go ahead and implement that soon. Would love to hear your feedback.

tanner0101 commented 7 years ago

@TG908 if you have time, implementing that as a PR into this all-resolved-addresses branch would be great. Otherwise, I will take this task when I have time.

Thanks!

tgymnich commented 7 years ago

Done #139

tgymnich commented 7 years ago

I can't figure out why the tests fail on the ci. They all succeed on my local machine. I dont think we even modified any files that the failed test depends on.

/home/ubuntu/sockets/Tests/SocketsTests/SelectTests.swift:24: error: 
SelectTests.testOnePipeReadyToWrite : XCTAssertEqual failed: ("0") is not equal to ("1") - Wrong write count

fatal error: Index out of range
tanner0101 commented 7 years ago

I've fixed the spacing (looks like some things became tab spaced making it look like more code was changed) and also made the tests not crash w/ index out of bounds.

I'm also seeing this issue locally on Sierra w/ Swift 4. Maybe it's a Swift 4 only issue?

tanner0101 commented 7 years ago

It appears the issue only happens when the tests are all run at the same time. They pass when run separately.

tgymnich commented 7 years ago

I think I figured it out #141