Closed Bug 1573564 Opened 3 months ago Closed 3 months ago

`fetchConfig` tries to await the DNS lookup

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: neil, Assigned: neil)

References

(Regression)

Details

Attachments

(1 file, 1 obsolete file)

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.

Attached patch Proposed patchSplinter Review
Assignee: nobody → neil
Attachment #9085130 - Flags: review?(mkmelin+mozilla)
Attachment #9085130 - Flags: review?(ben.bucksch)
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.
```
Attachment #9085130 - Flags: feedback+
No longer blocks: 1349337
Regressed by: 1349337
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.
Attachment #9085130 - Flags: review?(mkmelin+mozilla)
Attachment #9085130 - Flags: review?(ben.bucksch)
Attachment #9085130 - Flags: review+

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

Keywords: checkin-needed

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.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6c065f72218c
Make DNS lookup completely asynchronous. r=BenB,pmorris

Status: NEW → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0

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

Folded patch as requested.

Attachment #9085403 - Flags: feedback?(ben.bucksch)

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?

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

Attachment #9085403 - Attachment is obsolete: true
Attachment #9085403 - Flags: feedback?(ben.bucksch)

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

mercurial.ini

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

Thanks, I'll try that.

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

You need to log in before you can comment on or make changes to this bug.