dns resolver: invoke resolve callback if not cancelled#10060
dns resolver: invoke resolve callback if not cancelled#10060junr03 merged 2 commits intoenvoyproxy:masterfrom junr03:destroy-callback
Conversation
Signed-off-by: Jose Nino <jnino@lyft.com>
| // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the | ||
| // callback_ target _should_ still be around. In that case, raise the callback_ so the target | ||
| // can be done with this ActiveQuery and initiate a new one. | ||
| if (!cancelled_) { |
There was a problem hiding this comment.
This approach relies on the fact that all callback targets (strict_dns_cluster, logical_dns_cluster, redis_cluster, dns_cache) cancel active queries in their destructors. This behavior is already used when a PendingResolution successfully completes. However, this is not an enforced contract.
I was debating about going further and move this to a similar pattern as the one in the init manager where the lifetimes of the target and the watcher are independent of each other and it is safe to call functions/callbacks on each other regardless of them being freed.
There was a problem hiding this comment.
I think this is fine but if you want to add a TODO to clean this up, up to you.
Signed-off-by: Jose Nino <jnino@lyft.com>
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks for fixing!
| // ARES_ECONNREFUSED. If the PendingResolution has not been cancelled that means that the | ||
| // callback_ target _should_ still be around. In that case, raise the callback_ so the target | ||
| // can be done with this ActiveQuery and initiate a new one. | ||
| if (!cancelled_) { |
There was a problem hiding this comment.
I think this is fine but if you want to add a TODO to clean this up, up to you.
Description: Bump the Envoy ref. This PR brings in the relevant updates to Envoy Mobile:
envoyproxy/envoy#10060 which Fixes #687
Risk Level: low
Testing: unit test in Envoy, Envoy Mobile local build and CI
Fixes #687
Signed-off-by: Jose Nino <jnino@lyft.com>
Description:
PendingResolutions get destroyed when complete or when c-ares sentARES_EDESTRUCTION. Prior to #9899ARES_EDESTRUCTIONonly happened when the resolver was destroyed. However, after #9899 the channel, and thusPendingResolutions can be destroyed, without the callback target being aware. This leads to potential use after free issues. This PR calls the resolve callback when the status received isARES_EDESTRUCTION.Risk Level: med
Testing: added test that reproduced segfault and passed after fix.
Signed-off-by: Jose Nino jnino@lyft.com