1173709 - TelemetrySend.jsm should not wait on sending pings during |setup|

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

TelemetrySend should be changed so it doesn't wait on sending persisted pings during the setup [1].

TelemetryController.cleanupOnShutdown should also tell TelemetrySend to shutdown first, so that pings submitted during shutdown get persisted to disk.

[1] - https://hg.mozilla.org/mozilla-central/annotate/95afddf894e3/toolkit/components/telemetry/TelemetrySend.jsm#l267

Comment 1

[Alessio Placitelli [:Dexter]]

Missing background: previously, TelemetryController.jsm aborted outstanding pending ping requests when shutting down, before waiting on the barriers. This behaviour was changed by bug 1156359, when refactoring the logic into TelemetrySend.jsm. The result is that the client might get stuck when shutting down, waiting on pings to be sent.

I stumbled upon this issue while working on bug 1120379 (see the bc1 failures in [2]).

[1] - https://hg.mozilla.org/mozilla-central/annotate/032fa84aba3d/toolkit/components/telemetry/TelemetryController.jsm#l1050
[2] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb8fff1734b6

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1173709.patch

This patch makes sure TelemetryController shuts down TelemetrySend as the first thing during shutdown and doesn't make Telemetry block on persisted ping sending when starting up.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8620942 [details] [diff] [review]
bug1173709.patch

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +268,3 @@
>  
>      // Check the pending pings on disk now.
>      yield this._checkPendingPings();

_checkPendingPings() triggers ping sends too.
We could make this return a bool for "haveOverDuePings" and trigger sending - non-blockingly - from here.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1173709.patch - v2

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix failing tests

This patch fixes the tests failing because we're not blocking any more on sending persistent pings at startup.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8621529 [details] [diff] [review]
bug1173709.patch - v2

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +268,5 @@
>  
>      // Check the pending pings on disk now.
> +    let haveOverduePings = yield this._checkPendingPings();
> +    if (haveOverduePings) {
> +      this._sendPersistedPings();

Also comment here that not waiting is intentional.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8621551 [details] [diff] [review]
part 2 - Fix failing tests

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +211,5 @@
> +
> +  /**
> +   * Only used in tests to wait on overdue pings sent at startup.
> +   */
> +  testSendOverduePromise: function() {

I don't think we need should make this specific about overdue pings.
We add track ping activity via _trackPendingPingTask, so we can add a testWaitOnOutgoingPings() or so that waits on that.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Don't wait on sending pending pings - v3

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix failing tests - v2

As discussed over IRC, this patch also removes the connections barrier and tracks the pending ping activity manually. This also helps with waiting on pending ping activity in tests.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8622437 [details] [diff] [review]
part 2 - Fix failing tests - v2

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +224,5 @@
>    _pingSendTimer: null,
>    // This tracks all pending ping requests to the server.
>    _pendingPingRequests: new Map(),
> +  // This tracks all the pending async ping activity.
> +  _trackedPendingPings: new Map(),

_pendingPingActivity?

@@ +710,5 @@
>     */
>    _trackPendingPingTask: function (promise) {
> +    let clear = () => this._trackedPendingPings.delete(promise);
> +    promise.then(clear, clear);
> +    this._trackedPendingPings.set(promise, promise);

So we can just use a set here?

@@ +718,5 @@
> +   * Return a promise that allows to wait on pending pings.
> +   * @return {Object<Promise>} A promise resolved when all the pending pings promises
> +   *         are resolved.
> +   */
> +  _pendingPingsPromise: function () {

_promisePendingPingActivity()?

@@ +722,5 @@
> +  _pendingPingsPromise: function () {
> +    this._log.trace("_pendingPingsPromise - Waiting for ping task");
> +    // Pick an arbitrary element in the map, if any exists.
> +    let promiseEntry = this._trackedPendingPings.keys().next();
> +    if (promiseEntry.done) {

What is the keys.next part supposed to achieve?

We should only have active entries here, so:

#1 If the map is empty, we are done:
if (this._trackedPendingPings.size == 0) {
  ...

#2 Otherwise we need to wait on all the entries:
return Promise.all([for (p of ...) p.catch(...dummy)]);

#2 probably handles the case #1 covers just fine, so i guess we can leave out #1.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix failing tests - v3

Thanks! Changed addressing the comments.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8622457 [details] [diff] [review]
part 2 - Fix failing tests - v3

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

r=me with the below properly fixed.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +212,5 @@
> +  /**
> +   * Only used in tests to wait on outgoing pending pings.
> +   */
> +  testWaitOnOutgoingPings: function() {
> +    return TelemetrySendImpl._promisePendingPingActivity();

We use the `_` prefix here to mark "internal" functions.
_promisePendingPingActivity() is not only used internally, so it shouldn't be prefixed.

@@ +709,5 @@
>     * This is needed to block shutdown on any outstanding ping activity.
>     */
>    _trackPendingPingTask: function (promise) {
> +    let clear = () => this._pendingPingActivity.delete(promise);
> +    promise.then(clear, clear);

That doesn't work - .then() returns a new promise that you don't store anywhere.
Just do `...add(promise.then(clear, clear))` below.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix failing tests - v4

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> @@ +709,5 @@
> >     * This is needed to block shutdown on any outstanding ping activity.
> >     */
> >    _trackPendingPingTask: function (promise) {
> > +    let clear = () => this._pendingPingActivity.delete(promise);
> > +    promise.then(clear, clear);
> 
> That doesn't work - .then() returns a new promise that you don't store
> anywhere.
> Just do `...add(promise.then(clear, clear))` below.

As discussed over IRC, this is correct and this behaviour should be kept: the clear function would still look for the original promise and nothing would get removed from the promise list otherwise.

Comment 15

[Alessio Placitelli [:Dexter]]

Try push (along with bug 1120379): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f427e390220

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a8a08a33b4bf
https://hg.mozilla.org/integration/fx-team/rev/cc9acd02073c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8a08a33b4bf
https://hg.mozilla.org/mozilla-central/rev/cc9acd02073c