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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Depends on: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [b5] [unifiedTelemetry] )

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Assignee: nobody → alessio.placitelli
Blocks: 1156359
Blocks: 1120356
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
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
(Assignee)

Comment 2

3 years ago
Created attachment 8620942 [details] [diff] [review]
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.
Attachment #8620942 - Flags: review?(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.
Attachment #8620942 - Flags: review?(gfritzsche)
(Assignee)

Comment 4

3 years ago
Created attachment 8621529 [details] [diff] [review]
bug1173709.patch - v2
Attachment #8620942 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8621529 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

3 years ago
Created attachment 8621551 [details] [diff] [review]
part 2 - Fix failing tests

This patch fixes the tests failing because we're not blocking any more on sending persistent pings at startup.
Attachment #8621551 - Flags: review?(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.
Attachment #8621529 - Flags: review?(gfritzsche) → review+
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.
Attachment #8621551 - Flags: review?(gfritzsche)
(Assignee)

Comment 8

3 years ago
Created attachment 8621571 [details] [diff] [review]
part 1 - Don't wait on sending pending pings - v3
Attachment #8621529 - Attachment is obsolete: true
Attachment #8621571 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8622437 [details] [diff] [review]
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.
Attachment #8621551 - Attachment is obsolete: true
Attachment #8622437 - Flags: review?(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.
Attachment #8622437 - Flags: review?(gfritzsche)
(Assignee)

Comment 11

3 years ago
Created attachment 8622457 [details] [diff] [review]
part 2 - Fix failing tests - v3

Thanks! Changed addressing the comments.
Attachment #8622437 - Attachment is obsolete: true
Attachment #8622457 - Flags: review?(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.
Attachment #8622457 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8622498 [details] [diff] [review]
part 2 - Fix failing tests - v4
Attachment #8622457 - Attachment is obsolete: true
Attachment #8622498 - Flags: review+
(Assignee)

Comment 14

3 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/a8a08a33b4bf
https://hg.mozilla.org/mozilla-central/rev/cc9acd02073c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [uplift2]
Whiteboard: [uplift2] → [b5] [unifiedTelemetry] [uplift2]
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry]
Depends on: 1173961
You need to log in before you can comment on or make changes to this bug.