Based on my reading of the reference docs for erase, this line removes the resolved host from the hosts to drain vector and if it was the last host in the vector, we return early? That doesn't seem right to me since for example if we only had a single host, we'd still want to drain its connections. Unless I'm misunderstanding when erase returns zero.

You're correct that this would be wrong if this was a vector, but hosts_to_drain_ is an absl::flat_hash_set.

Here is what is being called:
https://github.com/abseil/abseil-cpp/blob/master/absl/container/flat_hash_set.h#L232

Another way to read this is "return if nothing was erased".

ok thanks for sharing that. I still haven't come around to appreciating this overall C++ pattern of returning iterators from functions, but this isn't the place to debate that.

I agree working with iterators is a bit of a mental adjustment. That said, in this case, the function is simply returning the number of elements that were deleted as a size_type. :)

should we explicitly empty hosts_to_drain_ before we start adding hosts here? I think that we cannot end up in a state where we have dups in here but what if we had a host in the past that does not exist anymore (not sure whether this can happen)?

Imagine we had DNS refresh 1) that included host x and y and DNS refresh 2) that included y host only. We want hosts_to_drain_ to contain only y after the second DNS refresh starts

I'm not actually sure if hosts can ever cease to exist in Envoy's cache implementation, since at the end of their TTL, I think they're simply refreshed. That said, previous re-resolutions should still exist even if this is triggered again before prior resolutions complete - in other words, I think the race where a host might persist exists irrespective of whether we call clear here.

In the case you describe:
The resolutions from 1) should still exist and be outstanding even if 2) is triggered, and thus we should still observe resolutions for x and y.

Is it possible that 1) DNS refresh is cancelled and we never receive a completion callback associated with it?

My understanding is the callback we're listening for should fire regardless of the result state of the resolution. @mattklein123 added it because other callbacks didn't necessarily.

should we explicitly empty hosts_to_drain_ before we start adding hosts here? I think that we cannot end up in a state where we have dups in here but what if we had a host in the past that does not exist anymore (not sure whether this can happen)?

Imagine we had DNS refresh 1) that included host x and y and DNS refresh 2) that included y host only. We want hosts_to_drain_ to contain only y after the second DNS refresh is initialized

hosts_to_drain_ is a set, and should not contain dups. I don't think emptying the set in here or above actually closes the window on the small window for a race where a host ceases to exist between us enumerating hosts and then triggering the refresh.

I think putting a time bound on outstanding resolutions would guard against a potential leak caused by that race, but that adds additional complexity, and given the narrow window, I'm not sure if it's really needed.

Is it possible that 1) DNS refresh is cancelled and we never receive a completion callback associated with it?