942317 - Experiment to test reachability of SRV records

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[u408661]

Attachment: dns_experiment.patch

See how feasible it is to use this as an upgrade path from http/1 to http/2. Windows-only, requires server-side support, as well.

Try run for existing patch at https://tbpl.mozilla.org/?tree=Try&rev=2b66bc998c61 (I wouldn't be surprised if some test somewhere is broken by this, but it at least worked on my Win7 VM)

Comment 1

[Patrick McManus [:mcmanus]]

Comment on attachment 8337041 [details] [diff] [review]
dns_experiment.patch

Review of attachment 8337041 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/dns/nsHostResolver.cpp
@@ +887,5 @@
> +        endALookup = mozilla::TimeStamp::Now();
> +
> +        // SRV lookup
> +        startSRVLookup = mozilla::TimeStamp::Now();
> +        srvStatus = dnsQuery(srvName.get(), DNS_TYPE_SRV, DNS_QUERY_STANDARD,

I think the A and SRV need to be done in parallel.. its unclear if there will need to be multiple round trips to pick up the delegations of org and mozilla and aus and its not fair to make the first lookup pay the cost of that and have the second lookup get the cache hit on it.

::: toolkit/components/telemetry/Histograms.json
@@ +1729,5 @@
> +  "SRV_EXPERIMENT_SUCCESS_DELTA": {
> +      "kind": "linear",
> +      "high": "2000",
> +      "n_buckets": 200,
> +      "description": "Time delta between A and SRV records when both succeed"

description should indicate the offset by 1k magic

Comment 2

[Patrick McManus [:mcmanus]]

also should have mentioned that the test should verify the expected return values when it succeeds.

Comment 3

[u408661]

Attachment: dns_experiment.patch

New version, all feedback comments addressed. Try seems unreasonably happy (which always kind of worries me), but hey, It Works On My Machine!(TM)

https://tbpl.mozilla.org/?tree=Try&rev=1685d80b2793

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8342068 [details] [diff] [review]
dns_experiment.patch

Review of attachment 8342068 [details] [diff] [review]:
-----------------------------------------------------------------

sweet 

needs to be conditional on 
1] pref network.allow-experiments 
2] Telemetry::CanRecord()
3] a new kill switch just for this feature. default to true for EARLY_BETA_OR_EARLIER, false otherwise

::: netwerk/dns/nsHostResolver.cpp
@@ +846,5 @@
> +    NS_IMETHOD Run() MOZ_OVERRIDE
> +    {
> +        nsAutoCString name;
> +        if (mQueryType == DNS_TYPE_SRV) {
> +            name.Assign(NS_LITERAL_CSTRING("srv-"));

"_http2tls.srv-"

@@ +974,5 @@
> +                                                          &srvResults,
> +                                                          dnsQuery),
> +                                   NS_DISPATCH_NORMAL);
> +
> +        WaitForMultipleObjects(2, events, TRUE, INFINITE);

mad windows skillz

@@ +985,5 @@
> +        }
> +
> +        if (srvStatus == DNS_RCODE_NOERROR) {
> +            DNS_SRV_DATA *srvData = &srvResults->Data.Srv;
> +            if (strcmp(srvData->pNameTarget, "success.http2test.mozilla.org") ||

strcasecmp

@@ +1066,5 @@
> +        MutexAutoLock lock(mExperimentLock);
> +        if (mHasRunExperiment) {
> +            return;
> +        }
> +

so the threading rules around the dns library are kind of squishy because it is pretty well locked. I think your new code requires the main thread (ns_newnamedthread() likely does).. so let's just do if (!IsMainThread()) return; right here in runexperiment().

@@ +1087,5 @@
>  {
>      MOZ_EVENT_TRACER_WAIT(rec, "net::dns::resolve");
>  
> +#if defined(XP_WIN)
> +    if (strcmp(rec->host, "aus.mozilla.org") == 0) {

I think we can do "ends in .mozilla.org" instead.. that might get us more data. Also preface the string operation with a racy if (enabled && !mHasRunExperiment) check.. (enabled based on the prefs) you'll need to do it again inside the lock, but that will avoid the string op and lock acquisition the vast majority of the time and always on release.

Comment 5

[Patrick McManus [:mcmanus]]

and stick a reference to this bug # in a comment to help with any wtf moments.

Comment 6

[u408661]

Attachment: dns_experiment.patch

All comments addressed. Fun fact: strcasecmp doesn't exist on windows, but _stricmp does. /me sighs

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8342679 [details] [diff] [review]
dns_experiment.patch

Review of attachment 8342679 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. exciting!

Comment 8

[u408661]

green try run: https://tbpl.mozilla.org/?tree=Try&rev=f8c210a749c6 (combine with try run from comment 3 for full greenness, since none of this code is even built outside windows)

Just waiting on inbound to open...

Comment 9

[u408661]

And inbound is open: remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0128d154ed3

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0128d154ed3

Comment 11

[Jacek Caban]

Trivial fix for cross compilation on case sensitive OSes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/316ab101c526

Comment 12

[Jason Duell]

Also landed on b2g-inbound:

  https://hg.mozilla.org/integration/b2g-inbound/rev/cf2a20bbc6cf

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf2a20bbc6cf

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/316ab101c526

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(disabled in 28 and later in bug 957759)