1344737 - Update TelemetrySession.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%3ATelemetrySession.jsm+Task.+yield&redirect=false

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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

Comment 2

[Swastikshrivastava0]

@Georg I would like to work on this. I've read the code and doc of async function. Can you let me know how to proceed further?

Comment 3

[Deepjyoti Mondal]

Attachment: bug1344737.patch

Updated TelemetrySession.jsm to async and await.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8846390 [details] [diff] [review]
bug1344737.patch

I'm short on time, redirecting request.

Comment 5

[Alessio Placitelli [:Dexter]]

Chris, would you mind reviewing this bug?

Comment 6

[Chris H-C :chutten]

Comment on attachment 8846390 [details] [diff] [review]
bug1344737.patch

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

Thank you for your patch!

Have you run the telemetry tests with this patch? From your mozilla-central root, run mach test toolkit/components/telemetry/tests/unit/. It appears as though you may have changed all of the `yield` to `await` where you instead should have only changed the ones next to functions that you changed from `Task.*` to `async function`.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1532,3 @@
>          // Update the session data to keep track of new subsessions created before
>          // the initialization.
> +        await TelemetryStorage.saveSessionData(this._getSessionDataObject());

If the TelemetryStorage function has not been updated to async, this won't play nicely, I don't think. Best to leave this untouched and let bug 1344743 take care of making this change.

@@ +1543,4 @@
>          Telemetry.asyncFetchTelemetryData(function() {});
>  
>          // Update the crash annotation with the proper client ID.
> +        annotateCrashReport(this._sessionId, await ClientID.getClientID(),

If ClientID hasn't been updated to async function, then await won't work.

@@ +2044,4 @@
>     * @return {Promise<object>} A promise which is resolved with an object when
>     *                            loading has completed, with null otherwise.
>     */
> +  async _loadSessionData() {

Is this valid syntax?

Comment 7

[Deepjyoti Mondal]

@Chris I have run unit tests from mozilla-central root, all the tests are passing

I had to use short hand notation for async function as otherwise I was getting esling error :

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions#Shorthand_async_methods

Comment 8

[Chris H-C :chutten]

Interesting. I wonder if it all works because of the format of the Promises returned by those embedded functions.

Let me throw this up on try (our build+test farm) and see if it can find any holes in it. Clearly I need more experience with these new asynchronous features :S

The try build will progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d73689619fa643de119775d06f91d92ac429b49

If it can't find anything, then it'll be an r+ and a checkin-needed from me.

Comment 9

[Chris H-C :chutten]

Comment on attachment 8846390 [details] [diff] [review]
bug1344737.patch

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

try's green, and I have some reading to do so I can learn why :S

Comment 10

[Chris H-C :chutten]

Thank you very much!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d762468d4657
Update TelemetrySession.jsm to async function & await. r=chutten

Comment 12

[Carsten Book [:Tomcat]]

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