Fix PingFoundation.swift compilation errors on macOS#6
Conversation
Co-authored-by: dustturtle <2305214+dustturtle@users.noreply.github.com>
Co-authored-by: dustturtle <2305214+dustturtle@users.noreply.github.com>
Co-authored-by: dustturtle <2305214+dustturtle@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces the prior TCP-based “ICMP” probe fallback with real ICMP echo request/reply implementations (Swift + Objective-C) and updates tests/docs accordingly, while also addressing macOS compilation issues in the new Swift PingFoundation implementation.
Changes:
- Implement real ICMP ping in Swift (
PingFoundation+ICMPPingerintegration). - Add Objective-C ICMP ping foundation/helper and wire
RRReachabilityto use it. - Add new unit tests for the ping components and update README to describe “real ICMP” behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/RealReachability2Tests/RealReachability2Tests.swift | Adds basic initialization/state tests for Swift PingFoundation. |
| Tests/RealReachability2ObjCTests/RRReachabilityTests.m | Adds ObjC tests for RRPingFoundation and RRPingHelper. |
| Sources/RealReachability2ObjC/include/RealReachability2ObjC.h | Exposes new ObjC ping headers publicly. |
| Sources/RealReachability2ObjC/include/RRPingHelper.h | Declares a block-based ping helper API for ObjC. |
| Sources/RealReachability2ObjC/include/RRPingFoundation.h | Declares low-level ObjC ICMP ping foundation + delegate API. |
| Sources/RealReachability2ObjC/RRReachability.m | Switches the ObjC probe from TCP connect to RRPingHelper (real ICMP). |
| Sources/RealReachability2ObjC/RRPingHelper.m | Implements the ObjC helper around RRPingFoundation. |
| Sources/RealReachability2ObjC/RRPingFoundation.m | Implements the ObjC ICMP ping foundation (SimplePing-style). |
| Sources/RealReachability2/Prober/PingFoundation.swift | Adds Swift ICMP ping foundation and fixes macOS compile issues noted in the PR description. |
| Sources/RealReachability2/Prober/ICMPPinger.swift | Replaces TCP-based probe with a ping operation using PingFoundation. |
| README.md | Updates documentation to describe real ICMP ping and configuration notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await withCheckedContinuation { continuation in | ||
| // Use a dedicated class to manage the ping operation | ||
| let pingOperation = PingOperation(host: host, timeout: timeout) | ||
| pingOperation.ping { success in | ||
| continuation.resume(returning: success) |
There was a problem hiding this comment.
PingOperation is created as a local inside withCheckedContinuation and is not retained after the closure returns. Because PingFoundation.delegate is weak and the Timer closure captures self weakly, the operation can be deallocated immediately, which can leave the continuation never resumed (hung probe). Retain the operation until completion (e.g., store it on ICMPPinger/a shared set, or intentionally self-retain during the ping and break the cycle in finishWithResult).
| let bytesRead = withUnsafeMutablePointer(to: &addr) { addrPtr in | ||
| addrPtr.withMemoryRebound(to: sockaddr.self, capacity: 1) { sockaddrPtr in | ||
| recvfrom(CFSocketGetNative(socket!), &buffer, buffer.count, 0, sockaddrPtr, &addrLen) |
There was a problem hiding this comment.
recvfrom is passed &buffer, which is a pointer to the Array value, not a pointer to the array’s contiguous element storage. This can lead to memory corruption / undefined behavior. Use buffer.withUnsafeMutableBytes { recvfrom(..., $0.baseAddress, ...) } (or withUnsafeMutableBufferPointer) to pass a valid writable buffer pointer.
| let bytesRead = withUnsafeMutablePointer(to: &addr) { addrPtr in | |
| addrPtr.withMemoryRebound(to: sockaddr.self, capacity: 1) { sockaddrPtr in | |
| recvfrom(CFSocketGetNative(socket!), &buffer, buffer.count, 0, sockaddrPtr, &addrLen) | |
| let bytesRead = buffer.withUnsafeMutableBytes { rawBuffer in | |
| withUnsafeMutablePointer(to: &addr) { addrPtr in | |
| addrPtr.withMemoryRebound(to: sockaddr.self, capacity: 1) { sockaddrPtr in | |
| recvfrom( | |
| CFSocketGetNative(socket!), | |
| rawBuffer.baseAddress, | |
| rawBuffer.count, | |
| 0, | |
| sockaddrPtr, | |
| &addrLen | |
| ) | |
| } |
| } | ||
|
|
||
| if bytesRead > 0 { | ||
| var packet = Data(bytes: buffer, count: bytesRead) |
There was a problem hiding this comment.
Data(bytes: buffer, count: bytesRead) is not a safe way to build Data from a [UInt8] buffer; it either won’t compile or will copy the wrong memory depending on overload resolution. Construct the Data from the buffer’s element storage (for example via Data(buffer.prefix(bytesRead)) or buffer.withUnsafeBytes { Data($0.prefix(bytesRead)) }).
| var packet = Data(bytes: buffer, count: bytesRead) | |
| var packet = Data(buffer.prefix(bytesRead)) |
| // Don't close socket on invalidate (we manage it) | ||
| let flags = CFSocketGetSocketFlags(socket) | ||
| CFSocketSetSocketFlags(socket, flags & ~CFOptionFlags(kCFSocketCloseOnInvalidate)) | ||
|
|
There was a problem hiding this comment.
The code clears kCFSocketCloseOnInvalidate (“Don’t close socket on invalidate”), but stop() only invalidates the CFSocket and never closes the native fd. This will leak file descriptors on each start/stop cycle. Either keep kCFSocketCloseOnInvalidate enabled or explicitly close(CFSocketGetNative(socket)) during teardown.
| let type = icmpData[0] | ||
| let code = icmpData[1] | ||
| let receivedIdentifier = UInt16(icmpData[4]) << 8 | UInt16(icmpData[5]) | ||
| let receivedSequence = UInt16(icmpData[6]) << 8 | UInt16(icmpData[7]) | ||
|
|
||
| guard type == ICMPv4Type.echoReply.rawValue else { return false } | ||
| guard code == 0 else { return false } | ||
| guard receivedIdentifier == identifier else { return false } | ||
| guard validateSequenceNumber(receivedSequence) else { return false } |
There was a problem hiding this comment.
IPv4 response validation doesn’t verify the ICMP checksum. The Objective-C implementation validates the checksum, and without it this can accept corrupted/unrelated packets that happen to match type/code/identifier/sequence. Consider validating the checksum for IPv4 replies (zero out the checksum field, compute, then compare to the received value) before treating the response as successful.
| // CFSocket will now close the socket | ||
| CFSocketSetSocketFlags(self.socket, CFSocketGetSocketFlags(self.socket) & ~(CFOptionFlags)kCFSocketCloseOnInvalidate); | ||
|
|
There was a problem hiding this comment.
kCFSocketCloseOnInvalidate is cleared here, but -stop only invalidates the CFSocket and never closes the native socket fd, causing an fd leak across repeated pings. Either leave kCFSocketCloseOnInvalidate enabled or explicitly close(fd)/close(CFSocketGetNative(self.socket)) during teardown.
| }]; | ||
| } | ||
|
|
||
| - (void)setHost:(NSString *)host { |
There was a problem hiding this comment.
-setHost: is declared/implemented as taking a nonnull NSString *, but the property is nullable in the header. This is a nullability contract mismatch and can produce warnings/unsound assumptions. Update the setter signature to accept a nullable NSString * _Nullable and handle nil consistently (or make the property nonnull if that’s the intent).
| - (void)setHost:(NSString *)host { | |
| - (void)setHost:(NSString * _Nullable)host { |
|
|
||
| @synchronized(self) { | ||
| for (RRPingCompletionBlock completion in self.completionBlocks) { | ||
| completion(NO, self.timeout); |
There was a problem hiding this comment.
pingTimeOut reports completion(NO, self.timeout), which contradicts the header contract (“Latency is 0 if ping failed”) and differs from endWithFlag:NO (which uses 0). Align timeout failure latency with the documented behavior (or update docs) to keep the API consistent.
| completion(NO, self.timeout); | |
| completion(NO, 0); |
| if (completion) { | ||
| @synchronized(self) { | ||
| [self.completionBlocks addObject:[completion copy]]; | ||
| } | ||
| } | ||
|
|
||
| if (!self.isPinging) { | ||
| // Must ensure pingFoundation runs on main thread | ||
| __weak typeof(self) weakSelf = self; | ||
| if (![[NSThread currentThread] isMainThread]) { | ||
| dispatch_sync(dispatch_get_main_queue(), ^{ | ||
| __strong typeof(weakSelf) strongSelf = weakSelf; | ||
| [strongSelf startPing]; | ||
| }); | ||
| } else { | ||
| [self startPing]; | ||
| } | ||
| } |
There was a problem hiding this comment.
pingWithBlock: checks isPinging outside of any synchronization. Because probes can be initiated concurrently (for example via RRReachability’s concurrent probeQueue), two threads can both observe isPinging == NO and race to start multiple pings / timers, leading to inconsistent completion behavior. Protect the isPinging check+transition with the same lock used for completionBlocks (or use an atomic/serial queue).
| if (completion) { | |
| @synchronized(self) { | |
| [self.completionBlocks addObject:[completion copy]]; | |
| } | |
| } | |
| if (!self.isPinging) { | |
| // Must ensure pingFoundation runs on main thread | |
| __weak typeof(self) weakSelf = self; | |
| if (![[NSThread currentThread] isMainThread]) { | |
| dispatch_sync(dispatch_get_main_queue(), ^{ | |
| __strong typeof(weakSelf) strongSelf = weakSelf; | |
| [strongSelf startPing]; | |
| }); | |
| } else { | |
| [self startPing]; | |
| } | |
| } | |
| BOOL shouldStartPing = NO; | |
| @synchronized(self) { | |
| if (completion) { | |
| [self.completionBlocks addObject:[completion copy]]; | |
| } | |
| if (!self.isPinging) { | |
| self.isPinging = YES; | |
| shouldStartPing = YES; | |
| } | |
| } | |
| if (!shouldStartPing) { | |
| return; | |
| } | |
| // Must ensure pingFoundation runs on main thread | |
| __weak typeof(self) weakSelf = self; | |
| if (![[NSThread currentThread] isMainThread]) { | |
| dispatch_sync(dispatch_get_main_queue(), ^{ | |
| __strong typeof(weakSelf) strongSelf = weakSelf; | |
| [strongSelf startPing]; | |
| }); | |
| } else { | |
| [self startPing]; | |
| } |
| - **Hybrid Approach**: Combines NWPathMonitor, HTTP HEAD, and real ICMP Ping for accurate reachability detection | ||
| - **Dual Target Support**: | ||
| - Swift version (iOS 13+) with async/await API | ||
| - Objective-C version (iOS 12+) with notification-based API | ||
| - **Configurable**: Choose between parallel, HTTP-only, or ICMP-only probe modes | ||
| - **True Reachability**: Verifies actual internet connectivity, not just network presence | ||
| - **Real ICMP Ping**: Uses actual ICMP echo request/reply (based on Apple's SimplePing) instead of TCP fallback |
There was a problem hiding this comment.
The PR title/description states this is a macOS compilation fix for PingFoundation.swift, but the diff also introduces a new Objective-C ICMP implementation (RRPingFoundation/RRPingHelper) and replaces the previous TCP-based probe logic. Please update the PR description/title to reflect the functional change, or split the compilation fix into a separate PR to keep review scope manageable.
CI was failing due to two compilation errors in the new
PingFoundation.swiftICMP implementation.Changes
CFStreamErrorDomain.netDB→kCFStreamErrorDomainNetDB: The Swift enum doesn't expose this member; use the C constant directlyprivate→fileprivatefordidFail(withHostStreamError:): Function is called from file-scopehostResolveCallback, requiresfileprivateaccessOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.