1573564 - `fetchConfig` tries to await the DNS lookup

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[[e-mail]]

All of the tasks fetchConfig spawns should run asynchronously and then the PriorityAbortable will then pick the best result. However to be able to do this properly all of the individual calls need to be added to it before any of them complete.

Bug 1349337 regressed this by making fetchConfig await the DNS lookup.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Paul Morris [:pmorris]]

Comment on attachment 9085130 [details] [diff] [review]
Proposed patch

Thanks for catching and fixing this.  The patch looks good to me.  This obsolete comment from the `getMX` function doc should be removed:
```
 * The promise-based async call to `DNS.mx` (imported from DNS.jsm) is at odds
 * with the other code here that works with callbacks and `Abortable` objects.
 * That makes the code less coherent, but otherwise it just means we can't
 * abort the initial MX query.
```

Comment 3

[Ben Bucksch (:BenB)]

Comment on attachment 9085130 [details] [diff] [review]
Proposed patch

r+ BenB

@Neil: Could you please make a diff to the state before bug 1349337? I'd like to verify that really only the implementation of `getMX()` has changed.

Comment 4

[Ben Bucksch (:BenB)]

(The review marker in the hg header is of course outdated now.)

Comment 5

[Jorg K (CEST = GMT+2)]

I can fix that. In the future, please supply patches with 8 lines of context. Strangely the patch had a minor complaint when applying it: Hunk #2 succeeded at 413 with fuzz 1 (offset -5 lines). So I'm really wondering what codebase you're working on. In such cases I usually compare the original and refresh patches, but here it leads nowhere since the original doesn't have 8 lines of context.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6c065f72218c
Make DNS lookup completely asynchronous. r=BenB,pmorris

Comment 7

[[e-mail]]

(In reply to Jorg K from comment #5)

In the future, please supply patches with 8 lines of context.

I guess mq ignores my -U 8 default?

So I'm really wondering what codebase you're working on.

I was working on commit ef868ddafb00 but my mq had my patch to bug 1572467 applied (since it makes the bug more obvious), so you would have different results depending on whether that one hadn't landed or both it and its dependency landed.

Comment 8

[[e-mail]]

Attachment: Fold of this fix and original DNS patch for comparison

Folded patch as requested.

Comment 9

[Jorg K (CEST = GMT+2)]

mercurial.ini

[diff]
git = 1
showfunc = 1
unified = 8

What's the new patch about? And requested where? I'm confused. I thought this bug was done?

Comment 10

[Ben Bucksch (:BenB)]

@Jörg: I requested this patch in comment 3. It was for informational purposes only.

Comment 11

[[e-mail]]

(In reply to Jorg K (GMT+2) from comment #9)

mercurial.ini

[diff]
git = 1
showfunc = 1
unified = 8

Thanks, I'll try that.

Comment 12

[Ben Bucksch (:BenB)]

No uplifts. Not applicable to 69 beta, 68 or 60, because it fixes a regression introduced by bug 1349337 in TB 70 only.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9085130 [details] [diff] [review]
Proposed patch

To go along with bug 1349337.

Comment 14

[Ben Bucksch (:BenB)]

@Magnus: Thanks that you thought of backporting this along with bug 1349337.

Comment 15

[Geoff Lankow (:darktrojan)]

68.3.1:
https://hg.mozilla.org/releases/comm-esr68/rev/dfeab0e6d5a43c6bcb1575bbdc5c0d018b42df9b