search

apple / swift-async-dns-resolver

A Swift library for asynchronous DNS requests, wrapping c-ares with Swift-friendly APIs and data structures.
Apache License 2.0
92 stars 16 forks source link

Make DNSSD queries cancellable #34

Open victorherrerod opened 6 months ago

victorherrerod commented 6 months ago

Resolves https://github.com/apple/swift-async-dns-resolver/issues/32

yim-lee commented 6 months ago

@swift-server-bot add to allowlist

victorherrerod commented 5 months ago

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 commented 5 months ago

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.

victorherrerod commented 5 months ago

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 commented 5 months ago

Thanks @victorherrerod.

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

victorherrerod commented 5 months ago

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.

victorherrerod commented 5 months ago

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.

victorherrerod commented 3 months ago

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 🙌