Skip to content

Make DNSSD queries cancellable #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Make DNSSD queries cancellable #34

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
610463d
Make DNSSD queries cancellable
victorherrerod Mar 22, 2024
a6299e5
Improve error handling and remove DNSServiceRef Sendable warning
victorherrerod Mar 22, 2024
de2fd16
Use DispatchReadSource to process DNSSD results
victorherrerod Apr 9, 2024
8be4b7d
Add Query class to handle queries' pointers
victorherrerod Apr 9, 2024
472d5c0
Fix formatting
victorherrerod Apr 9, 2024
6ad9bd9
Add cancel handler to manage resources instead of Query class
victorherrerod Apr 11, 2024
8102fb4
Deallocate pointers in error branches too
victorherrerod Apr 30, 2024
792d8e1
Convert deallocatePointers to static and use initialize(to:) for the …
victorherrerod May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 37 additions & 24 deletions Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,6 @@ struct DNSSD {
let recordStream = AsyncThrowingStream<ReplyHandler.Record, Error> { continuation in
let handler = QueryReplyHandler(handler: replyHandler, continuation)

// Wrap `handler` into a pointer so we can pass it to DNSServiceQueryRecord
let handlerPointer = UnsafeMutableRawPointer.allocate(
byteCount: MemoryLayout<QueryReplyHandler>.stride,
alignment: MemoryLayout<QueryReplyHandler>.alignment
)

handlerPointer.initializeMemory(as: QueryReplyHandler.self, repeating: handler, count: 1)

// The handler might be called multiple times so don't deallocate inside `callback`
defer {
let pointer = handlerPointer.assumingMemoryBound(to: QueryReplyHandler.self)
pointer.deinitialize(count: 1)
pointer.deallocate()
}

// This is called once per record received
let callback: DNSServiceQueryRecordReply = { _, _, _, errorCode, _, _, _, rdlen, rdata, _, context in
guard let handlerPointer = context else {
Expand All @@ -138,32 +123,54 @@ struct DNSSD {
handler.handleRecord(errorCode: errorCode, data: rdata, length: rdlen)
}

let serviceRefPtr = UnsafeMutablePointer<DNSServiceRef?>.allocate(capacity: 1)
defer { serviceRefPtr.deallocate() }
let serviceRefPointer = UnsafeMutablePointer<DNSServiceRef?>.allocate(capacity: 1)

// Wrap 'handler' into a pointer so we can pass it to DNSServiceQueryRecord
let replyHandlerPointer = UnsafeMutablePointer<QueryReplyHandler>.allocate(capacity: 1)
replyHandlerPointer.initialize(to: handler)

// Run the query
let _code = DNSServiceQueryRecord(
serviceRefPtr,
serviceRefPointer,
kDNSServiceFlagsTimeout,
0,
name,
UInt16(type.kDNSServiceType),
UInt16(kDNSServiceClass_IN),
callback,
handlerPointer
replyHandlerPointer
)

// Check if query completed successfully
guard _code == kDNSServiceErr_NoError else {
DNSSD.deallocatePointers(serviceRefPointer: serviceRefPointer, replyHandlerPointer: replyHandlerPointer)
return continuation.finish(throwing: AsyncDNSResolver.Error(dnssdCode: _code))
}

// Read reply from the socket (blocking) then call reply handler
DNSServiceProcessResult(serviceRefPtr.pointee)
DNSServiceRefDeallocate(serviceRefPtr.pointee)
let serviceSockFD = DNSServiceRefSockFD(serviceRefPointer.pointee)
guard serviceSockFD != -1 else {
DNSSD.deallocatePointers(serviceRefPointer: serviceRefPointer, replyHandlerPointer: replyHandlerPointer)
return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we leaking the serviceRefPointer and replyHandlerPointer in this branch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code so that the deallocates are also called in this branch and the other error branch just above.

}

let readSource = DispatchSource.makeReadSource(fileDescriptor: serviceSockFD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be passing our own queue here rather than using the default?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this file descriptor leaked?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I close it now in the cancellation handler

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll need .setCancelHandler here and manage the resources from there.

DispatchSources that use file descriptors and don't use setCancelHandler are guaranteed to be incorrect. Also see https://developer.apple.com/documentation/dispatch/1385648-dispatch_source_set_cancel_handl?language=objc#discussion

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for the feedback! I implemented it in the latest commit

readSource.setEventHandler {
// Read reply from the socket (blocking) then call reply handler
DNSServiceProcessResult(serviceRefPointer.pointee)

readSource.cancel()
continuation.finish()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should cancel the read source

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in latest commit

}

readSource.setCancelHandler {
DNSSD.deallocatePointers(serviceRefPointer: serviceRefPointer, replyHandlerPointer: replyHandlerPointer)
close(serviceSockFD)
}
readSource.resume()

// Streaming done
continuation.finish()
continuation.onTermination = { _ in
readSource.cancel()
}
}

// Build reply using records received
Expand All @@ -173,6 +180,12 @@ struct DNSSD {

return try replyHandler.generateReply(records: records)
}

private static func deallocatePointers(serviceRefPointer: UnsafeMutablePointer<DNSServiceRef?>, replyHandlerPointer: UnsafeMutablePointer<DNSSD.QueryReplyHandler>) {
DNSServiceRefDeallocate(serviceRefPointer.pointee)
serviceRefPointer.deallocate()
replyHandlerPointer.deallocate()
}
}

// MARK: - dnssd query reply handler
Expand Down