Skip to content
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.

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

Make DNSSD queries cancellable #32

Open
victorherrerod opened this issue Mar 18, 2024 · 5 comments · May be fixed by #34
Open

Make DNSSD queries cancellable #32

victorherrerod opened this issue Mar 18, 2024 · 5 comments · May be fixed by #34

Comments

@victorherrerod
Copy link

Hi!
First of all, thanks for this great lib!

I'd like to be able to cancel queries made with the DNSSD Resolver, because the timeout is pretty long and it would be nice to be able to cancel the Task after a custom timeout.
I've seen that the cancellation behavior is implemented for the Ares Resolver and I've tried to make it work for the DNSSD Resolver.
I implemented the onTermination method of the stream continuation, however this produces a warning when trying to call DNSServiceRefDeallocate on the pointer, since it isn't a Sendable type and it could be modified by several threads at the same time.

I am not very experienced in the inner workings of Swift concurrency, how could the DNSSD queries cancellation be implemented?

Thanks and have a nice day!

yim-lee added a commit to yim-lee/swift-async-dns-resolver that referenced this issue Mar 18, 2024
yim-lee added a commit to yim-lee/swift-async-dns-resolver that referenced this issue Mar 18, 2024
@yim-lee
Copy link
Member

yim-lee commented Mar 18, 2024

I don't see any warnings locally with #33. Which Swift version are you using? Would you be able to give changes in that PR a try to see if it works for you?

@victorherrerod
Copy link
Author

I'm using Swift 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4) and I get the warning Capture of 'serviceRefPtr' with non-sendable type 'UnsafeMutablePointer<DNSServiceRef?>' (aka 'UnsafeMutablePointer<Optional<OpaquePointer>>') in a ``@Sendable`` closure.

The changes in the PR don't seem to work. onTermination is called, but only after the long DNSSD timeout, and the termination is always .finished(nil), never .cancelled.
Here's a minimal example of code where my 3s timeout is not respected and the SRV query still runs for 10-15s:

import Foundation
import AsyncDNSResolver

struct TimeoutError: Error {}

func getSRVRecords(withTimeout: TimeInterval) async throws -> [SRVRecord] {
    let resolver = try AsyncDNSResolver()
    let records = try await withThrowingTaskGroup(of: [SRVRecord].self) { group in
        group.addTask {
            return try await resolver.querySRV(name: "")
        }
        group.addTask {
            try await Task.sleep(nanoseconds: UInt64(withTimeout) * NSEC_PER_SEC)
            try Task.checkCancellation()
            throw TimeoutError()
        }
        do {
            let records = try await group.next()!
            group.cancelAll()
            return records
        } catch {
            group.cancelAll()
            throw error
        }
    }
    return records
}

do {
    let records = try await getSRVRecords(withTimeout: 3)
    print(records)
} catch {
    print("Error: \(error)")
}

In my example I put "" as the service name, but I believe it happens with any SRV query that has no answer. group.cancelAll() is called when the query times out, in the catch, but onTermination is not called at that time.

@yim-lee
Copy link
Member

yim-lee commented Mar 19, 2024

DNSServiceProcessResult is blocking. We might have to do something like this SO suggested using select:

// Source: https://stackoverflow.com/questions/22502729/resolving-srv-records-with-ios-sdk

- (void)updateDnsRecords
{
    if (self.dnsUpdatePending == YES)
    {
        return;
    }
    else
    {
        self.dnsUpdatePending = YES;
    }

    NSLog(@"DNS update");
    DNSServiceRef       sdRef;
    DNSServiceErrorType err;

    const char* host = [self.dnsHost UTF8String];
    if (host != NULL)
    {
        NSTimeInterval remainingTime = self.dnsUpdateTimeout;
        NSDate*        startTime = [NSDate date];

        err = DNSServiceQueryRecord(&sdRef, 0, 0,
                                    host,
                                    kDNSServiceType_SRV,
                                    kDNSServiceClass_IN,
                                    processDnsReply,
                                    &remainingTime);

        // This is necessary so we don't hang forever if there are no results
        int            dns_sd_fd = DNSServiceRefSockFD(sdRef);
        int            nfds      = dns_sd_fd + 1;
        fd_set         readfds;
        int            result;

        while (remainingTime > 0)
        {
            FD_ZERO(&readfds);
            FD_SET(dns_sd_fd, &readfds);

            struct timeval tv;
            tv.tv_sec  = (time_t)remainingTime;
            tv.tv_usec = (remainingTime - tv.tv_sec) * 1000000;

            result = select(nfds, &readfds, (fd_set*)NULL, (fd_set*)NULL, &tv);
            if (result == 1)
            {
                if (FD_ISSET(dns_sd_fd, &readfds))
                {
                    err = DNSServiceProcessResult(sdRef);
                    if (err != kDNSServiceErr_NoError)
                    {
                        NSLog(@"There was an error reading the DNS SRV records.");
                        break;
                    }
                }
            }
            else if (result == 0)
            {
                NBLog(@"DNS SRV select() timed out");
                break;
            }
            else
            {
                if (errno == EINTR)
                {
                    NBLog(@"DNS SRV select() interrupted, retry.");
                }
                else
                {
                    NBLog(@"DNS SRV select() returned %d errno %d %s.", result, errno, strerror(errno));
                    break;
                }
            }

            NSTimeInterval elapsed = [[NSDate date] timeIntervalSinceDate:startTime];
            remainingTime -= elapsed;
        }

        DNSServiceRefDeallocate(sdRef);
    }
}

But instead of having a timeout/deadline we check for Task.isCancelled periodically? (thinking out loud)

The existing reply handling code should do what processDnsReply does already, so we shouldn't need to worry too much about it I think.

Hopefully the code involved (e.g., DNSServiceRefSockFD) works for all the query types this library supports and not just SRV. Looks like it is, but we will definitely need to test it.

Note that the referenced SO response is from 10 years ago. Maybe there are newer/better ways to do this.

@victorherrerod Is this something you want to try looking into?

@Lukasa
Copy link

Lukasa commented Mar 20, 2024

If you want to wake a select on cancellation the usual recommendation would be to also add an eventFD or a pipe to the fdset and write into it on cancellation, then check for that.

victorherrerod pushed a commit to victorherrerod/swift-async-dns-resolver that referenced this issue Mar 22, 2024
@victorherrerod victorherrerod linked a pull request Mar 22, 2024 that will close this issue
@victorherrerod
Copy link
Author

victorherrerod commented Mar 22, 2024

Instead of using select I used poll to avoid being blocked, as I've read it's more efficient. I thought about using an eventFD as @Lukasa mentioned, but I didn't know which library to import for get eventFD honestly (the Linux <sys/eventfd.h>) was not found in Xcode.

I've tested this a bit and it seems to work correctly, my code snippet above behaves as expected and the task cancels after 3 seconds.

The main doubt I have is removing DNSServiceRefDeallocate(serviceRefPtr.pointee) after calling DNSServiceProcessResult(serviceRefPtr.pointee). I did this because I have noticed the onTermination callback is always executed after calling continuation.finish(), even when there is no error, so I didn't want to call it twice when the request is successful.

However, I still have the Sendable warning I mentioned previously, I don't exactly know how to fix that.

PD: I amended the commit to have the author linked to my account

victorherrerod added a commit to victorherrerod/swift-async-dns-resolver that referenced this issue Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants