Update TelemetrySend.jsm to async function & await

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P4
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: gfritzsche, Assigned: Deepa, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

9.66 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 months ago
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.
(Assignee)

Comment 1

5 months ago
Hi Georg,

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

Thanks
Flags: needinfo?(gfritzsche)
(Reporter)

Updated

5 months ago
Assignee: nobody → deepsrijit1105
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 2

5 months ago
Created attachment 8844102 [details] [diff] [review]
WIP

Hi Georg,

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

Thanks
(Reporter)

Comment 3

5 months ago
What kind of failures is it causing?
Can you add some examples?
Flags: needinfo?(deepsrijit1105)
(Assignee)

Comment 4

5 months ago
Created attachment 8846061 [details] [diff] [review]
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
Attachment #8844102 - Attachment is obsolete: true
Flags: needinfo?(deepsrijit1105) → needinfo?(gfritzsche)
(Reporter)

Updated

5 months ago
Attachment #8846061 - Attachment is patch: true
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 5

5 months ago
(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?
Flags: needinfo?(deepsrijit1105)
(Assignee)

Comment 6

5 months ago
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?
Flags: needinfo?(deepsrijit1105) → needinfo?(gfritzsche)
(Reporter)

Comment 7

5 months ago
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.
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 8

5 months ago
Per the above, can you submit a patch with everything except that function changed?
Flags: needinfo?(deepsrijit1105)
(Assignee)

Comment 9

5 months ago
Created attachment 8848572 [details] [diff] [review]
final_patch

Hi Georg,

I have updated the patch.
Attachment #8846061 - Attachment is obsolete: true
Flags: needinfo?(deepsrijit1105) → needinfo?(gfritzsche)
Attachment #8848572 - Flags: review?(gfritzsche)
(Reporter)

Comment 10

5 months ago
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.
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 11

5 months ago
Comment on attachment 8848572 [details] [diff] [review]
final_patch

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

Thanks, that looks good!
Attachment #8848572 - Flags: review?(gfritzsche) → review+
(Reporter)

Updated

5 months ago
Keywords: checkin-needed

Comment 12

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f48633cdb53
Update TelemetrySend.jsm to async function & await. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5f48633cdb53
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.