Merge pull request #970 from brizental/1741898-a-throttling-bug

Bug 1741898 - Don't enqueue multiple stop command on uploader dispatcher

CHANGELOG.md

@@ -6,6 +6,7 @@
   * Users may provide a folder name through the `VIRTUAL_ENV` environment variable.
   * If the user is inside an active virtualenv the `VIRTUAL_ENV` environment variable is already set by Python. See: https://docs.python.org/3/library/venv.html.
 * [#968](https://github.com/mozilla/glean.js/pull/968): Add runtime arguments type checking to `Glean.setUploadEnabled` API.
+* [#970](https://github.com/mozilla/glean.js/pull/970): BUGFIX: Guarantee uploading is immediatelly resumed if the uploader has been stopped due to any of the uploading limits being hit.
 
 # v0.25.0 (2021-11-15)
 

glean/src/core/upload/index.ts

@@ -93,6 +93,8 @@ class PingUploader implements PingsDatabaseObserver {
   private readonly platformInfo: PlatformInfo;
   // The server address we are sending pings to.
   private readonly serverEndpoint: string;
+  // Whether or not uploading is currently stopped due to limits having been hit.
+  private stopped = false;
 
   constructor(
     config: Configuration,
@@ -132,17 +134,21 @@ class PingUploader implements PingsDatabaseObserver {
     const { state: rateLimiterState, remainingTime } = this.rateLimiter.getState();
     if (rateLimiterState === RateLimiterState.Incrementing) {
       this.dispatcher.resume();
+      this.stopped = false;
     } else {
-      // Stop the dispatcher respecting the order of the dispatcher queue,
-      // to make sure the Stop command is enqueued _after_ previously enqueued requests.
-      this.dispatcher.stop(false);
+      if(!this.stopped) {
+        // Stop the dispatcher respecting the order of the dispatcher queue,
+        // to make sure the Stop command is enqueued _after_ previously enqueued requests.
+        this.dispatcher.stop(false);
+        this.stopped = true;
+      }
 
       if (rateLimiterState === RateLimiterState.Throttled) {
         log(
           LOG_TAG,
           [
-            "Attempted to upload a ping, but Glean is currently throttled.",
-            `Pending pings will be processed in ${(remainingTime || 0) / 1000}s.`
+            "Succesfully submitted a ping, but Glean is currently throttled.",
+            `Pending pings may be uploaded in ${(remainingTime || 0) / 1000}s.`
           ],
           LoggingLevel.Debug
         );
@@ -151,9 +157,9 @@ class PingUploader implements PingsDatabaseObserver {
         log(
           LOG_TAG,
           [
-            "Attempted to upload a ping, but Glean has reached maximum recoverable upload failures",
-            "for the current uploading window.",
-            `Will retry in ${(remainingTime || 0) / 1000}s.`
+            "Succesfully submitted a ping,",
+            "but Glean has reached maximum recoverable upload failures for the current uploading window.",
+            `May retry in ${(remainingTime || 0) / 1000}s.`
           ],
           LoggingLevel.Debug
         );

glean/tests/unit/core/upload/index.spec.ts

@@ -25,7 +25,10 @@ const sandbox = sinon.createSandbox();
  * @param pingName the name of the ping to fill the database with, defaults to "ping".
  * @returns The array of identifiers of the pings added to the database.
  */
-async function fillUpPingsDatabase(numPings: number, pingName = "ping"): Promise<string[]> {
+async function fillUpPingsDatabase(
+  numPings: number,
+  pingName = "ping"
+): Promise<string[]> {
   const ping = new PingType({
     name: pingName,
     includeClientId: true,
@@ -246,4 +249,22 @@ describe("PingUploader", function() {
     assert.ok("X-Client-Version" in headers);
     assert.ok("X-Telemetry-Agent" in headers);
   });
+
+  it("dispatcher is only once stopped if upload limits are hit and is immediatelly restarted after", async function () {
+    const httpClient = new CounterUploader();
+    await Glean.testResetGlean(testAppId, true, { httpClient });
+
+    const stopSpy = sandbox.spy(Glean["pingUploader"]["dispatcher"], "stop");
+    await fillUpPingsDatabase((MAX_PINGS_PER_INTERVAL * 2) - 1);
+    await Glean["pingUploader"].testBlockOnPingsQueue();
+    assert.strictEqual(1, stopSpy.callCount);
+
+    // Reset the rate limiter so that we are not throttled anymore.
+    Glean["pingUploader"]["rateLimiter"]["reset"]();
+
+    // Add one more ping to the queue so that uploading is resumed.
+    await fillUpPingsDatabase(1);
+    await Glean["pingUploader"].testBlockOnPingsQueue();
+    assert.strictEqual(httpClient.count, MAX_PINGS_PER_INTERVAL * 2);
+  });
 });