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 gethostbyaddr fail with `ECANCELLED` for `ares_cancel()` #138

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@addaleax
Copy link
Contributor

commented Aug 14, 2017

When ares_cancel() was invoked, ares_gethostbyaddr() queries would fail with ENOTFOUND instead of ECANCELLED.

It seems appropriate to treat ares_cancel() like ares_destroy(), but I would appreciate review of the correctness of this change.

Ref: nodejs/node#14814

Make gethostbyaddr fail with `ECANCELLED` for `ares_cancel()`
When `ares_cancel()` was invoked, `ares_gethostbyaddr()`
queries would fail with `ENOTFOUND` instead of `ECANCELLED`.

It seems appropriate to treat `ares_cancel()` like `ares_destroy()`,
but I would appreciate review of the correctness of this change.

Ref: nodejs/node#14814
@dimbleby

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

If this is correct, then presumably ares_gethostbyname() should have a similar fix here.

@addaleax

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2017

@dimbleby I don’t think so, for ares_gethostbyname() the status is already passed along via next_lookup(); I assume there is a reason for that disparity in the first place.

@coveralls

This comment has been minimized.

Copy link

commented Aug 14, 2017

Coverage Status

Coverage remained the same at 95.382% when pulling e8a8637 on addaleax:cancel-ecancelled into 33bf5ba on c-ares:master.

@bagder

bagder approved these changes Aug 14, 2017

Copy link
Member

left a comment

I agree with this reasoning!

@addaleax

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2017

Bump, anything I can do for this to be merged? Should I ping the mailing list?

@bagder bagder closed this in 0ef4a0c Aug 24, 2017

@bagder

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

Thanks!

@addaleax addaleax deleted the addaleax:cancel-ecancelled branch Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.