Skip to content

Commit

Permalink
GetaddrinfoResolver succeeds futures on eventLoop
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 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 committed Jan 8, 2025
1 parent 100b1f4 commit cd63dc9
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 9 deletions.
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 cd63dc9

Please sign in to comment.