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
82 stars 12 forks source link

Simplify AsyncDNSResolver.Error #27

Closed glbrntt closed 4 months ago

glbrntt commented 4 months ago

Motivation:

The 'AsyncDNSResolver.Error' is closely modelled on the c-ares error codes and doesn't fit well with the DNSSD error codes. Further, the DNSSD backed doesn't map its error cdes back into an 'AsyncDNSResolver.Error' so all DNSSD errors become 'other' errors.

Modifications:

Result:

Error codes are more consistent across implementations.

glbrntt commented 4 months ago

@yim-lee AsyncDNSResolver.Error seems effectively modelled on errors from c-ares, so I'm not sure if this change is the right one.

An alternative could be having a minimal set of error codes on AsyncDNSResolver.Error (I'm not sure what that minimal set would be) and introduce a cause/underlyingError property (which would just be an Error?) which contains more specific information, in practice that would be a CARESError or a DNSSDError. I suspect we don't get much additional benefit from this though. WDYT?

yim-lee commented 4 months ago

AsyncDNSResolver.Error seems effectively modelled on errors from c-ares

Yeah, the DNSSD implementation was done much later and it was an oversight of mine that I didn't even think about mapping the kDNSServiceErr_ error codes.

An alternative could be having a minimal set of error codes on AsyncDNSResolver.Error (I'm not sure what that minimal set would be)

This might be a good idea. I do think that some of the error codes are too specific, that I am not even sure what some of them mean. The common list you have in this PR is a good starting point, although badFlags and notInitialized are more of library than user errors in our case, and I don't know if we want to distinguish that.

and introduce a cause/underlyingError property (which would just be an Error?) which contains more specific information, in practice that would be a CARESError or a DNSSDError. I suspect we don't get much additional benefit from this though.

I wonder if knowing whether the error comes from c-ares or DNSSD (i.e., which implementation the library is using) is useful information? If so, then having CARESError and DNSSDError might be beneficial, although I think it would also be fine if that's just a flag/property in AsyncDNSResolver.Errors.

So it might boil down to the following:

@glbrntt wdyt?

glbrntt commented 4 months ago

An alternative could be having a minimal set of error codes on AsyncDNSResolver.Error (I'm not sure what that minimal set would be)

This might be a good idea. I do think that some of the error codes are too specific, that I am not even sure what some of them mean. The common list you have in this PR is a good starting point, although badFlags and notInitialized are more of library than user errors in our case, and I don't know if we want to distinguish that.

Oh 100%, I was going on what sounded about right. The docs on the DNSSD error codes is... lacking.

and introduce a cause/underlyingError property (which would just be an Error?) which contains more specific information, in practice that would be a CARESError or a DNSSDError. I suspect we don't get much additional benefit from this though.

I wonder if knowing whether the error comes from c-ares or DNSSD (i.e., which implementation the library is using) is useful information? If so, then having CARESError and DNSSDError might be beneficial, although I think it would also be fine if that's just a flag/property in AsyncDNSResolver.Errors.

Most of the time they won't be but I think it's worth having when the error code makes no sense i.e. we didn't know what to map it to so we put it as e.g. code 'unknown', if that happens then the user should be able to retrieve the underlying code to debug any further should they need (or report back as an issue).

So it might boil down to the following:

  • Reduce the list of error codes to what you propose in this PR
  • Map c-ares and DNSSD errors to these codes
  • Add source enum property to indicate if error comes up c-ares/DNSSD

@glbrntt wdyt?

Yeah this sounds good although I think the source should include the underlying error code as well.

yim-lee commented 4 months ago

if that happens then the user should be able to retrieve the underlying code to debug any further should they need (or report back as an issue).

Yeah this sounds good although I think the source should include the underlying error code as well.

Yeah, makes sense. 👍