Alessio Placitelli [:Dexter] Assignee
	Description • 9 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

	Alessio Placitelli [:Dexter] Assignee
	Comment 1 • 9 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

	Alessio Placitelli [:Dexter] Assignee
	Comment 2 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 5 • 9 years ago

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

	Georg Fritzsche [:gfritzsche]
	Comment 6 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 7 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 9 • 9 years ago

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.

	Georg Fritzsche [:gfritzsche]
	Comment 10 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

Thanks! Changed addressing the comments.

	Georg Fritzsche [:gfritzsche]
	Comment 12 • 9 years ago

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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 14 • 9 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.

	Alessio Placitelli [:Dexter] Assignee
	Comment 15 • 9 years ago

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

	Pulsebot
	Comment 16 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

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