1344741 - Update TelemetrySend.jsm to async function & await

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Georg Fritzsche [:gfritzsche]]

We have `async function` and `await` now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

We should be able to replace:
- `Task.async(function*(...))`, `Task.spawn(function*(...))` with a form of `async function ...`
- `yield` with `await` in the changed functions

https://dxr.mozilla.org/mozilla-central/search?q=path%3ATelemetrySend.jsm+Task.&redirect=false

The only case that doesn't migrate trivially is any `DeferredTask` usage, we could just leave those for now.

Comment 1

[Deepa]

Hi Georg,

I would like to take this bug up, can you assign me this bug?

Thanks

Comment 2

[Deepa]

Attachment: WIP

Hi Georg,

This an intermediate patch. It's causing almost 26 fails while testing. I will ping you tomorrow in irc.

Thanks

Comment 3

[Georg Fritzsche [:gfritzsche]]

What kind of failures is it causing?
Can you add some examples?

Comment 4

[Deepa]

Attachment: updated patch

I have made some changes, which reduced the errors to 2. I now have 2 errors to be resolved. The log is here, http://pasted.co/d1a4255b
I cannot find the actual errors / causes. Can you help me?

Thanks,
Deepa

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Deepa from comment #4)
> I have made some changes, which reduced the errors to 2. I now have 2 errors
> to be resolved. The log is here, http://pasted.co/d1a4255b
> I cannot find the actual errors / causes. Can you help me?

The specific test failure is in only one test, reproduced with:
> mach test toolkit/components/telemetry/tests/unit/test_TelemetryController.js

If you look closer at the test output, you will see an error like:
> 0:04.97 TEST_STATUS: Thread-3 test_disableDataUpload FAIL [test_disableDataUpload : 181] All the pending pings but the deletion ping should have been deleted - 0 == 1
> ...

Now, this surprises me a bit because the patch here shouldn't actually really change the behavior.

Thinking about what could have changed here:
(1) behavior: Not likely, the patch seems like straight-forward replacements.
(2) timing: Again, not likely, the patch seems like straight-forward replacements.
(3) errors: I don't see any JavaScript errors in the output (e.g. by searching for "error" through the whole test output).

Ok, so we need to investigate a little more. Maybe it is one of (1) or (2)?
We can try to narrow this down by:
- undoing all the changes
- running the test to confirm its working now
- replacing one Task.async(), running the test again
  - if it succeeds, continue
  - if it fails, we narrowed the failure down to a specific function.

Can you try that?

Comment 6

[Deepa]

Yes, I did that. The problem is arising when I change clearCurrentPings() to async.
All other methods are all good after changes. Does this information help?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Thanks!
I dug a little into that from here and found that this runs into a race condition in the test.
We don't want to break anything important here by accident, so we want to take a little time to properly fix this.

As we are a work-week currently, we can't fix this right now.
So, lets move forward with this patch as-is and i file another bug on changing that function.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Per the above, can you submit a patch with everything except that function changed?

Comment 9

[Deepa]

Attachment: final_patch

Hi Georg,

I have updated the patch.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Thanks Deepa.
Just as a note: When you set the review flag i already get mail about it, so in that case there is no need to set the needinfo flag.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8848572 [details] [diff] [review]
final_patch

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

Thanks, that looks good!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f48633cdb53
Update TelemetrySend.jsm to async function & await. r=gfritzsche

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5f48633cdb53