Make DNSSD queries cancellable #34

victorherrerod · 2024-03-22T16:54:18Z

Resolves #32

Resolves apple#32

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

yim-lee · 2024-03-22T17:53:25Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+                if result == 0 {
+                    continue
+                }
+                if result == 1 {


Is it always 1 and not any positive number?

According to poll man page, it's the number of elements in the pollFDs array that are ready for I/O. The array being of size 1, it could not be anything other than 1 if it is a positive number.

I can change it to >=, but that should never happen.

yim-lee · 2024-03-22T17:56:29Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+            var pollFDs = [pollfd(fd: serviceSockFD, events: Int16(POLLIN), revents: 0)]
+            while true {
+                guard !Task.isCancelled else {
+                    return continuation.finish(throwing: CancellationError())


I wonder if it would be more useful to throw AsyncDNSResolver.Error here? (we would need to add .cancelled to the enum)

Yes I thought that CancellationError was the standard way to throw when a task is cancelled, but apparently it's not a guarantee and an AsyncDNSResolver.Error might be more specific and useful.

yim-lee · 2024-03-22T17:57:41Z

@swift-server-bot add to allowlist

Add AsyncDNSResolver.Error.cancelled and error messages when polling the DNSSD service socket

weissi

This looks like a kernel thread blocking function in the middle of an asynchronous function

weissi · 2024-03-25T09:58:48Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+                return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
+            }
+
+            var pollFDs = [pollfd(fd: serviceSockFD, events: Int16(POLLIN), revents: 0)]


poll is blocking until this is ready so we can't do this here. If you want async I/O you'd need to use SwiftNIO here.

We did work to remove the SwiftNIO dependency earlier (#20), so I don't think we want to add it back.

The intention here is to allow checking for task cancellation, because calling DNSServiceProcessResult alone just blocks and doesn't seem to give us the chance to do that at all.

The docs for DNSServiceProcessResult says:

This call will block until the daemon's response is received. Use DNSServiceRefSockFD() in conjunction with a run loop or select() to determine the presence of a response from the server before calling this function to process the reply without blocking.

That's why we went with this approach here. It doesn't sound like we must do async I/O to see improvement, but I could be wrong of course.

If poll isn't called with a timeout, it'll block. If we mustn't use NIO, prefer DispatchSources.

Use DNSServiceRefSockFD() in conjunction with a run loop

^^ that's exactly what NIO can do for you.

What's the goal with removing the NIO dependency? Every adopter will have a NIO dependency anyway, right?

What's the goal with removing the NIO dependency?

Only a couple of methods on ByteBuffer were being used for parsing so pulling in NIO was quite heavy handed. Replacing with ArraySlice<UInt8> was relatively straightforward. There's no requirement to be free of NIO.

Every adopter will have a NIO dependency anyway, right?

Not necessarily although I'm sure many will.

If poll isn't called with a timeout, it'll block.

We are passing 0 as timeout I believe (poll(&pollFDs, 1, 0)), so it should return immediately according to the man page:

Specifying a negative value in timeout means an infinite timeout. Specifying a timeout of zero causes poll() to return immediately, even if no file descriptors are ready.

I wonder if it should sleep a bit before calling poll again though.

that's exactly what NIO can do for you

Is there an example that we can follow? I found these threads:

https://forums.swift.org/t/socket-library-built-with-swift-concurrency/56505

Think about adding a poll-loop mode where Selectable EventLoop can be driven from other threads without taking them over swift-nio#2074

But I didn't find anything obvious.

I don't doubt that SwiftNIO can do this more efficiently, but at the end of the day we need to look at the trade-offs. Does doing it without SwiftNIO give us a "good enough" solution? Is it worthwhile to add SwiftNIO, which is a heavier dependency, for this? (I don't have problem with adding the SwiftNIO dependency FWIW.)

Hot-looping on poll is not the right choice: it's not meaningfully different than just sleeping the thread, as we'll never yield it.

Doing this with NIO directly is gonna be a little painful as you'd have to write a Channel to intercept the readFromSocket function. Not the end of the world, but not as easy as you might like. I continue to think that DispatchSource is going to be the easiest way to achieve this goal.

Oh yes, if you don't actually need to do anything with the data in there, then DispacthSource is good. It's pretty bad if you need to actually read the data but if you just need to be told that there is something to read, DispatchSource.makeReadSource is fine

Thanks for your feedback, I'm going to try implementing this with DispatchSource then, if it's ok for everyone.

Convert DNSSD to class to use deinit on the pointers we initialized for dns-sd C functions The Dispatch Source event handler is called when data can be read from the DNSSD service socket Remove the unchecked Sendable subtyping for the DNSServiceRef pointer as it's no longer needed Remove the cancelled case from the error code enum as it's no longer needed

victorherrerod · 2024-04-09T08:31:01Z

Hi, sorry for the wait, I updated the implementation to use DispatchReadSource to process the query results.

I converted the DNSSD struct to a class to use the deinit and deinitialize the pointers when we are done. The current implementation deinitialized the pointers at the end of the query function, which caused a crash when calling DNSServiceProcessResult from the event handler.
Let me know if converting it to a class is OK or if we should change it.

Other than that I only removed code that was no longer necessary.

victorherrerod · 2024-04-09T15:39:24Z

Don't bother reviewing for now as the code is actually not correct, we only have one reply handler pointer so concurrent requests cause a crash. Trying to fix it.

Convert DNSSD back to struct A different Query class is created for every query, so concurrent queries no longer cause an issue

victorherrerod · 2024-04-10T07:58:00Z

I added a Query class to handle the pointer initialization and deallocation, now a Query instance is created for every query made and the deallocation happens in the deinit of each instance.

I have tested with many concurrent requests and it seems to be working correctly. This implementation uses DispatchSource, as stated above.

Let me know if there is any changes I should make!

yim-lee · 2024-04-10T19:00:47Z

Thanks @victorherrerod.

@weissi @Lukasa @ktoso Can you please review the latest changes when you have time? Thanks in advance.

yim-lee · 2024-04-10T20:58:12Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+            byteCount: MemoryLayout<QueryReplyHandler>.stride,
+            alignment: MemoryLayout<QueryReplyHandler>.alignment
+        )
+        init(handler: QueryReplyHandler) {


nit: add newline before

yim-lee · 2024-04-10T21:00:15Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+        let replyHandlerPointer = UnsafeMutableRawPointer.allocate(
+            byteCount: MemoryLayout<QueryReplyHandler>.stride,
+            alignment: MemoryLayout<QueryReplyHandler>.alignment
+        )


Is there a difference in initializing serviceRefPtr and replyHandlerPointer here vs. inside the initializer? Does anyone have any preference?

Slight preference towards doing it in the init so the allocate and initializeMemory calls are next to each other.

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

glbrntt · 2024-04-11T07:15:48Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+        let replyHandlerPointer = UnsafeMutableRawPointer.allocate(
+            byteCount: MemoryLayout<QueryReplyHandler>.stride,
+            alignment: MemoryLayout<QueryReplyHandler>.alignment
+        )


Slight preference towards doing it in the init so the allocate and initializeMemory calls are next to each other.

glbrntt · 2024-04-11T07:26:41Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+                return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
+            }
+
+            let readSource = DispatchSource.makeReadSource(fileDescriptor: serviceSockFD)


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

weissi

security relevant use after free bug
plus: either file descriptor leak or use after free

weissi · 2024-04-11T14:23:39Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+
+            continuation.onTermination = { _ in
+                readSource.cancel()
+                DNSServiceRefDeallocate(query.serviceRefPtr.pointee)


security relevant use after free bug here, you could be concurrently deallocating this as well as using it in the event handler

The deallocation is now done in the dispatch source cancel handler so that should no longer happen

weissi · 2024-04-11T14:23:58Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

-            // Streaming done
-            continuation.finish()
+                // Streaming done
+                continuation.finish()


this should cancel the read source

Applied in latest commit

weissi · 2024-04-11T14:24:50Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+                return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
+            }
+
+            let readSource = DispatchSource.makeReadSource(fileDescriptor: serviceSockFD)


isn't this file descriptor leaked?

I close it now in the cancellation handler

weissi · 2024-04-11T14:25:55Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

+                return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
+            }
+
+            let readSource = DispatchSource.makeReadSource(fileDescriptor: serviceSockFD)


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

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

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

DNSServiceRefDeallocate is called in the DispatchSource cancellation handler to be sure it's not used in the event handler after being freed Remove Query class as it is not needed for pointer management Close DNSServiceRefSockFD in DispatchSource cancellation handler to avoid any FD leak

victorherrerod · 2024-04-11T16:38:12Z

Thanks everyone for the feedback, I followed @weissi advice and implemented a cancellation handler for the dispatch source that manages the ressources. I also deleted the Query class, which was kind of a hack to handle the pointers deallocation.

I have tested concurrent queries and everything seems to work correctly. Unit tests all passed too.
Let me know if there are other changes that have to be made.

glbrntt · 2024-04-19T08:51:06Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

-            DNSServiceRefDeallocate(serviceRefPtr.pointee)
+            let serviceSockFD = DNSServiceRefSockFD(serviceRefPointer.pointee)
+            guard serviceSockFD != -1 else {
+                return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))


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

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

victorherrerod · 2024-05-07T13:06:40Z

Hi, could someone review the changes? We'd like to use this in a project to update our SRV resolver to use async/await, and being able to timeout the requests by cancelling them is necessary.

I've tried to answer and fix most of the issues, let me know if there is anything else that's needed.

Lukasa · 2024-05-10T12:41:11Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

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


Let's avoid initialize(repeating:), it's not needed. Just initialize(to:) is fine.

Fixed in latest commit

Lukasa · 2024-05-10T12:45:24Z

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift

@@ -173,6 +180,12 @@ struct DNSSD {

        return try replyHandler.generateReply(records: records)
    }
+
+    private func deallocatePointers(serviceRefPointer: UnsafeMutablePointer<DNSServiceRef?>, replyHandlerPointer: UnsafeMutablePointer<DNSSD.QueryReplyHandler>) {


Making this static would make the code a lot easier to follow.

I made it static in the latest commit

…reply handler pointer

victorherrerod · 2024-06-07T11:46:45Z

Anything new for this PR? It would be nice to be able to use it in a project, and with the requests being uncancellable and the timeout being so long, it's kind of difficult to use.
I understand that it's still a pretty new lib that may not be prod-ready, so no pressure 🙌

XfableX · 2025-01-15T13:22:16Z

Hey all any updates to this one? Would love to use this in some projects but the un-cancellable 30 second timeout is brutal.

Make DNSSD queries cancellable

610463d

Resolves apple#32

victorherrerod force-pushed the dnssd-cancellation branch from 072927e to 610463d Compare March 22, 2024 17:46

yim-lee reviewed Mar 22, 2024

View reviewed changes

yim-lee requested review from weissi, Lukasa and ktoso March 22, 2024 17:58

Improve error handling and remove DNSServiceRef Sendable warning

a6299e5

Add AsyncDNSResolver.Error.cancelled and error messages when polling the DNSSD service socket

weissi requested changes Mar 25, 2024

View reviewed changes

victorherrerod added 2 commits April 9, 2024 18:46

Add Query class to handle queries' pointers

8be4b7d

Convert DNSSD back to struct A different Query class is created for every query, so concurrent queries no longer cause an issue

Fix formatting

472d5c0

yim-lee requested review from weissi and glbrntt April 10, 2024 19:00

yim-lee reviewed Apr 10, 2024

View reviewed changes

glbrntt reviewed Apr 11, 2024

View reviewed changes

weissi requested changes Apr 11, 2024

View reviewed changes

weissi reviewed Apr 11, 2024

View reviewed changes

Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift Outdated Show resolved Hide resolved

yim-lee requested review from weissi and glbrntt April 18, 2024 15:50

glbrntt reviewed Apr 19, 2024

View reviewed changes

Deallocate pointers in error branches too

8102fb4

Lukasa reviewed May 10, 2024

View reviewed changes

Convert deallocatePointers to static and use initialize(to:) for the …

792d8e1

…reply handler pointer

gmilos mentioned this pull request Jun 4, 2024

Queries for non-existent record type take long time to eventually time out #37

Open

Make DNSSD queries cancellable #34

Are you sure you want to change the base?

Make DNSSD queries cancellable #34

Uh oh!

Conversation

victorherrerod commented Mar 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee commented Mar 22, 2024

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glbrntt Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorherrerod commented Apr 9, 2024

Uh oh!

victorherrerod commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victorherrerod commented Apr 10, 2024

Uh oh!

yim-lee commented Apr 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

glbrntt Mar 25, 2024 •

edited

Loading

victorherrerod commented Apr 9, 2024 •

edited

Loading