Skip to content

Commit

Permalink
GetaddrinfoResolver succeeds futures on eventLoop (#3042)
Browse files Browse the repository at this point in the history
### Motivation:

`testClientBindWorksOnSocketsBoundToEitherIPv4OrIPv6Only` would fail
sometimes leaking the IPv4 promise in `GetaddrinfoResolver`

`HappyEyeballsConnector` returns the connection when it resolves either
IPv4 of IPv6. It uses the `GetaddrinfoResolver` which holds a promise
for each of the IPv4 and IPv6 resolution; when one is completed the
connection will be returned and it is possible to start tearing down the
test and shutting down the event loop before the other is completed and
we leak the promise.

### Modifications:

Complete both futures on the event loop rather than the dispatch queue.

### Result:

The futures are completed in the same event loop tick meaning that we
cannot continue execution and leak one.
  • Loading branch information
rnro authored Jan 8, 2025
1 parent 4292153 commit 27c839f
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 14 deletions.
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 555
"mallocCountTotal" : 107
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 556
"mallocCountTotal" : 109
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 548
"mallocCountTotal" : 107
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 548
"mallocCountTotal" : 107
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"mallocCountTotal" : 548
"mallocCountTotal" : 107
}
4 changes: 2 additions & 2 deletions Sources/NIOPosix/Bootstrap.swift
Original file line number Diff line number Diff line change
Expand Up @@ -956,8 +956,6 @@ public final class ClientBootstrap: NIOClientTCPBootstrapProtocol {
///
/// Using `bind` is not necessary unless you need the local address to be bound to a specific address.
///
/// - Note: Using `bind` will disable Happy Eyeballs on this `Channel`.
///
/// - Parameters:
/// - address: The `SocketAddress` to bind on.
public func bind(to address: SocketAddress) -> ClientBootstrap {
Expand All @@ -978,6 +976,8 @@ public final class ClientBootstrap: NIOClientTCPBootstrapProtocol {

/// Specify the `host` and `port` to connect to for the TCP `Channel` that will be established.
///
/// - Note: Makes use of Happy Eyeballs.
///
/// - Parameters:
/// - host: The host to connect to.
/// - port: The port to connect to.
Expand Down
19 changes: 14 additions & 5 deletions Sources/NIOPosix/GetaddrinfoResolver.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import struct WinSDK.SOCKADDR_IN6
let offloadQueueTSV = ThreadSpecificVariable<DispatchQueue>()

internal class GetaddrinfoResolver: Resolver {
private let loop: EventLoop
private let v4Future: EventLoopPromise<[SocketAddress]>
private let v6Future: EventLoopPromise<[SocketAddress]>
private let aiSocktype: NIOBSDSocket.SocketType
Expand All @@ -67,6 +68,7 @@ internal class GetaddrinfoResolver: Resolver {
aiSocktype: NIOBSDSocket.SocketType,
aiProtocol: NIOBSDSocket.OptionLevel
) {
self.loop = loop
self.v4Future = loop.makePromise()
self.v6Future = loop.makePromise()
self.aiSocktype = aiSocktype
Expand All @@ -90,7 +92,6 @@ internal class GetaddrinfoResolver: Resolver {
/// Initiate a DNS AAAA query for a given host.
///
/// Due to the nature of `getaddrinfo`, we only actually call the function once, in this function.
/// That means this function call actually blocks: sorry!
///
/// - Parameters:
/// - host: The hostname to do an AAAA lookup on.
Expand Down Expand Up @@ -214,16 +215,24 @@ internal class GetaddrinfoResolver: Resolver {
info = nextInfo
}

v6Future.succeed(v6Results)
v4Future.succeed(v4Results)
// Ensure that both futures are succeeded in the same tick
// to avoid racing and potentially leaking a promise
self.loop.execute {
self.v6Future.succeed(v6Results)
self.v4Future.succeed(v4Results)
}
}

/// Record an error and fail the lookup process.
///
/// - Parameters:
/// - error: The error encountered during lookup.
private func fail(_ error: Error) {
self.v6Future.fail(error)
self.v4Future.fail(error)
// Ensure that both futures are succeeded in the same tick
// to avoid racing and potentially leaking a promise
self.loop.execute {
self.v6Future.fail(error)
self.v4Future.fail(error)
}
}
}
4 changes: 2 additions & 2 deletions Tests/NIOPosixTests/BootstrapTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ class BootstrapTest: XCTestCase {
// Some platforms don't define "localhost" for IPv6, so check that
// and use "ip6-localhost" instead.
if !isIPv4 {
let hostResolver = GetaddrinfoResolver(loop: group.next(), aiSocktype: .stream, aiProtocol: .tcp)
let hostResolver = GetaddrinfoResolver(loop: self.group.next(), aiSocktype: .stream, aiProtocol: .tcp)
let hostv6 = try! hostResolver.initiateAAAAQuery(host: "localhost", port: 8088).wait()
if hostv6.isEmpty {
localhost = "ip6-localhost"
Expand All @@ -752,7 +752,7 @@ class BootstrapTest: XCTestCase {
XCTFail("can't connect channel 1")
return
}
XCTAssertEqual(localIP, maybeChannel1?.localAddress?.ipAddress)
XCTAssertEqual(localIP, myChannel1Address.ipAddress)
// Try 3: Bind the client to the same address/port as in try 2 but to server 2.
XCTAssertNoThrow(
try ClientBootstrap(group: self.group)
Expand Down

0 comments on commit 27c839f

Please sign in to comment.