DnsOverHttps Fixups: event sequencing, resolvePublicAddresses, IPv6 first#9553
Conversation
| } | ||
|
|
||
| override fun isCanceled() = state.get() is State.Canceled | ||
| override fun isCanceled() = calls.all { it.isCanceled() } |
There was a problem hiding this comment.
I think given RealCall, this works in practice.
But based on the interface of Call, it could be racy?
/** Cancels the request, if possible. Requests that are already complete cannot be canceled. */
fun cancel()
So if some are completed, and you are using some other Call impl, which I know is unlikely, it may not be correct when one call already completed?
There was a problem hiding this comment.
Ooooh, yeah I totally missed that. Good analysis. I added a test case that handles RealCall, but it’s arguably a bug that RealCall allows you to cancel something that’s already completed.
There was a problem hiding this comment.
And I added a volatile to track whether this is canceled, similar to how RealCall does it. I previously had a Canceled state (and LookupDnsCall still does!), so I might refactor to do that also.
yschimke
left a comment
yschimke
left a comment
There was a problem hiding this comment.
one question about cancellation
2 participants
The biggest change here is we’re now requesting TYPE_AAAA (IPv6) records first in all DnsOverHttps calls. This should be a negligible behavior change for most applications as the two calls can execute in parallel. The order only really matters when there’s a limit on the total concurrency against the DNS service. (This is the case for our tests, which only permit 1 call at a a time.)
Fix some bugs in how events are delivered to the callback. Always deliver events on OkHttp’s dispatcher thread. This includes cancel events and resolvePublicAddress validation. Otherwise it’s surprising if the callback is sometimes invoked by the thread calling
enqueue().