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 1 commit
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
29 changes: 28 additions & 1 deletion Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ struct DNSSD {
let serviceRefPtr = UnsafeMutablePointer<DNSServiceRef?>.allocate(capacity: 1)
defer { serviceRefPtr.deallocate() }

continuation.onTermination = { _ in
DNSServiceRefDeallocate(serviceRefPtr.pointee)
}

// Run the query
let _code = DNSServiceQueryRecord(
serviceRefPtr,
Expand All @@ -158,9 +162,32 @@ struct DNSSD {
return continuation.finish(throwing: AsyncDNSResolver.Error(dnssdCode: _code))
}

let serviceSockFD = DNSServiceRefSockFD(serviceRefPtr.pointee)
guard serviceSockFD != -1 else {
return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError))
}

var pollFDs = [pollfd(fd: serviceSockFD, events: Int16(POLLIN), revents: 0)]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@glbrntt glbrntt Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:

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.)

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

while true {
guard !Task.isCancelled else {
return continuation.finish(throwing: CancellationError())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

}

let result = poll(&pollFDs, 1, 0)
guard result != -1 else {
return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError))
}

if result == 0 {
continue
}
if result == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Is it always 1 and not any positive number?

Copy link
Author

Choose a reason for hiding this comment

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

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.

break
}
}

// Read reply from the socket (blocking) then call reply handler
DNSServiceProcessResult(serviceRefPtr.pointee)
DNSServiceRefDeallocate(serviceRefPtr.pointee)

// Streaming done
continuation.finish()
Expand Down