From 27c839f4700069928196cd0e9fa03b22f297078a Mon Sep 17 00:00:00 2001 From: Rick Newton-Rogers Date: Wed, 8 Jan 2025 16:33:33 +0000 Subject: [PATCH] GetaddrinfoResolver succeeds futures on eventLoop (#3042) ### 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. --- .../5.10/NIOPosixBenchmarks.TCPEcho.p90.json | 2 +- .../5.9/NIOPosixBenchmarks.TCPEcho.p90.json | 2 +- .../6.0/NIOPosixBenchmarks.TCPEcho.p90.json | 2 +- .../NIOPosixBenchmarks.TCPEcho.p90.json | 2 +- .../NIOPosixBenchmarks.TCPEcho.p90.json | 2 +- Sources/NIOPosix/Bootstrap.swift | 4 ++-- Sources/NIOPosix/GetaddrinfoResolver.swift | 19 ++++++++++++++----- Tests/NIOPosixTests/BootstrapTest.swift | 4 ++-- 8 files changed, 23 insertions(+), 14 deletions(-) diff --git a/Benchmarks/Thresholds/5.10/NIOPosixBenchmarks.TCPEcho.p90.json b/Benchmarks/Thresholds/5.10/NIOPosixBenchmarks.TCPEcho.p90.json index d7233c85b9..c9e4dbd4c6 100644 --- a/Benchmarks/Thresholds/5.10/NIOPosixBenchmarks.TCPEcho.p90.json +++ b/Benchmarks/Thresholds/5.10/NIOPosixBenchmarks.TCPEcho.p90.json @@ -1,3 +1,3 @@ { - "mallocCountTotal" : 555 + "mallocCountTotal" : 107 } diff --git a/Benchmarks/Thresholds/5.9/NIOPosixBenchmarks.TCPEcho.p90.json b/Benchmarks/Thresholds/5.9/NIOPosixBenchmarks.TCPEcho.p90.json index 8517c2fe45..82d63a322b 100644 --- a/Benchmarks/Thresholds/5.9/NIOPosixBenchmarks.TCPEcho.p90.json +++ b/Benchmarks/Thresholds/5.9/NIOPosixBenchmarks.TCPEcho.p90.json @@ -1,3 +1,3 @@ { - "mallocCountTotal" : 556 + "mallocCountTotal" : 109 } diff --git a/Benchmarks/Thresholds/6.0/NIOPosixBenchmarks.TCPEcho.p90.json b/Benchmarks/Thresholds/6.0/NIOPosixBenchmarks.TCPEcho.p90.json index 67abcf36ac..c9e4dbd4c6 100644 --- a/Benchmarks/Thresholds/6.0/NIOPosixBenchmarks.TCPEcho.p90.json +++ b/Benchmarks/Thresholds/6.0/NIOPosixBenchmarks.TCPEcho.p90.json @@ -1,3 +1,3 @@ { - "mallocCountTotal" : 548 + "mallocCountTotal" : 107 } diff --git a/Benchmarks/Thresholds/nightly-6.0/NIOPosixBenchmarks.TCPEcho.p90.json b/Benchmarks/Thresholds/nightly-6.0/NIOPosixBenchmarks.TCPEcho.p90.json index 67abcf36ac..c9e4dbd4c6 100644 --- a/Benchmarks/Thresholds/nightly-6.0/NIOPosixBenchmarks.TCPEcho.p90.json +++ b/Benchmarks/Thresholds/nightly-6.0/NIOPosixBenchmarks.TCPEcho.p90.json @@ -1,3 +1,3 @@ { - "mallocCountTotal" : 548 + "mallocCountTotal" : 107 } diff --git a/Benchmarks/Thresholds/nightly-main/NIOPosixBenchmarks.TCPEcho.p90.json b/Benchmarks/Thresholds/nightly-main/NIOPosixBenchmarks.TCPEcho.p90.json index 67abcf36ac..c9e4dbd4c6 100644 --- a/Benchmarks/Thresholds/nightly-main/NIOPosixBenchmarks.TCPEcho.p90.json +++ b/Benchmarks/Thresholds/nightly-main/NIOPosixBenchmarks.TCPEcho.p90.json @@ -1,3 +1,3 @@ { - "mallocCountTotal" : 548 + "mallocCountTotal" : 107 } diff --git a/Sources/NIOPosix/Bootstrap.swift b/Sources/NIOPosix/Bootstrap.swift index 20a0a4844d..6d1312b6ae 100644 --- a/Sources/NIOPosix/Bootstrap.swift +++ b/Sources/NIOPosix/Bootstrap.swift @@ -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 { @@ -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. diff --git a/Sources/NIOPosix/GetaddrinfoResolver.swift b/Sources/NIOPosix/GetaddrinfoResolver.swift index 7a17589fb0..b24b0a9a2f 100644 --- a/Sources/NIOPosix/GetaddrinfoResolver.swift +++ b/Sources/NIOPosix/GetaddrinfoResolver.swift @@ -51,6 +51,7 @@ import struct WinSDK.SOCKADDR_IN6 let offloadQueueTSV = ThreadSpecificVariable() internal class GetaddrinfoResolver: Resolver { + private let loop: EventLoop private let v4Future: EventLoopPromise<[SocketAddress]> private let v6Future: EventLoopPromise<[SocketAddress]> private let aiSocktype: NIOBSDSocket.SocketType @@ -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 @@ -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. @@ -214,8 +215,12 @@ 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. @@ -223,7 +228,11 @@ internal class GetaddrinfoResolver: Resolver { /// - 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) + } } } diff --git a/Tests/NIOPosixTests/BootstrapTest.swift b/Tests/NIOPosixTests/BootstrapTest.swift index 8c64a578e3..309f8b1c81 100644 --- a/Tests/NIOPosixTests/BootstrapTest.swift +++ b/Tests/NIOPosixTests/BootstrapTest.swift @@ -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" @@ -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)