Skip to content

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Feb 19, 2024

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:

  • Reduce the number of error codes in 'AsyncDNSResolver.Error' to a few
    high level error codes and a catch-all 'internal' error code.
  • Update mappings from c-ares error codes, add mappings from dnssd error
    codes.
  • Add a 'source' property, which can be 'any Error', allowing users to
    get fine grained error information if necessary.

Result:

Error codes are more consistent across implementations.

@glbrntt
Copy link
Contributor Author

glbrntt commented Feb 19, 2024

@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
Copy link
Member

yim-lee commented Feb 19, 2024

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:

  • 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?

@glbrntt
Copy link
Contributor Author

glbrntt commented Mar 4, 2024

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
Copy link
Member

yim-lee commented Mar 4, 2024

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

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:

- Reduce the number of error codes in 'AsyncDNSResolver.Error' to a few
  high level error codes and a catch-all 'internal' error code.
- Update mappings from c-ares error codes, add mappings from dnssd error
  codes.
- Add a 'source' property, which can be 'any Error', allowing users to
  get fine grained error information if necessary.

Result:

Error codes are more consistent across implementations.
@glbrntt glbrntt changed the title Add mappings for DNSSD error codes Simplify AsyncDNSResolver.Error Mar 5, 2024
@glbrntt glbrntt marked this pull request as ready for review March 5, 2024 15:13
@glbrntt glbrntt requested a review from yim-lee March 5, 2024 15:13
Copy link
Member

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

❤️

@glbrntt glbrntt merged commit 08c07ff into apple:main Mar 6, 2024
@glbrntt glbrntt deleted the dnssd-error branch March 6, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants