Skip to content

DnsOverHttps Fixups: event sequencing, resolvePublicAddresses, IPv6 first#9553

Open
swankjesse wants to merge 3 commits into
masterfrom
jwilson.0716.new_tests
Open

DnsOverHttps Fixups: event sequencing, resolvePublicAddresses, IPv6 first#9553
swankjesse wants to merge 3 commits into
masterfrom
jwilson.0716.new_tests

Conversation

@swankjesse

Copy link
Copy Markdown
Collaborator

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().

}

override fun isCanceled() = state.get() is State.Canceled
override fun isCanceled() = calls.all { it.isCanceled() }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

@yschimke yschimke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one question about cancellation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants