Merge branch 'main' into bug_1702206

Author: ChinYing-Li

CHANGELOG.md

@@ -1,9 +1,15 @@
 # Unreleased changes
 
-[Full changelog](https://github.com/mozilla/glean.js/compare/v0.18.0...main)
+[Full changelog](https://github.com/mozilla/glean.js/compare/v0.18.1...main)
 
 * [#534](https://github.com/mozilla/glean.js/pull/534): Expose `Uploader` base class through `@mozilla/glean/<platform>/uploader` entry point.
 
+# v0.18.1 (2021-07-22)
+
+[Full changelog](https://github.com/mozilla/glean.js/compare/v0.18.0...v0.18.1)
+
+* [#552](https://github.com/mozilla/glean.js/pull/552): BUGFIX: Do not clear `deletion-request` ping from upload queue when disabling upload.
+
 # v0.18.0 (2021-07-20)
 
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.17.0...v0.18.0)

glean/package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@mozilla/glean",
-  "version": "0.18.0",
+  "version": "0.18.1",
   "description": "An implementation of the Glean SDK, a modern cross-platform telemetry client, for JavaScript environments.",
   "type": "module",
   "sideEffects": "false",

glean/package.json

@@ -8,7 +8,7 @@ export const GLEAN_SCHEMA_VERSION = 1;
 //
 // PACKAGE_VERSION is defined as a global by webpack,
 // we need a default here for testing when the app is not build with webpack.
-export const GLEAN_VERSION = "0.18.0";
+export const GLEAN_VERSION = "0.18.1";
 
 // The name of a "ping" that will include Glean ping_info metrics,
 // such as ping sequence numbers.

glean/src/core/constants.ts

@@ -27,18 +27,27 @@ export const enum DispatcherState {
 
 // The possible commands to be processed by the dispatcher.
 const enum Commands {
-  // The dispatcher must enqueue a new task.
+  // The dispatcher will enqueue a new task.
   //
   // This command is always followed by a concrete task for the dispatcher to execute.
   Task,
+  // Same as the `Task` command,
+  // but the task enqueued by this command is not cleared by the `Clear` command.
+  //
+  // # Note
+  //
+  // `Shutdown` will still clear these tasks.
+  //
+  // Unless unavoidable, prefer using the normal `Task`.
+  PersistentTask,
   // The dispatcher should stop executing the queued tasks.
   Stop,
   // The dispatcher should stop executing the queued tasks and clear the queue.
   Clear,
   // The dispatcher will clear the queue and go into the Shutdown state.
   Shutdown,
   // Exactly like a normal Task, but spawned for tests.
-  TestTask,
+  TestTask = "TestTask",
 }
 
 // A task the dispatcher knows how to execute.
@@ -47,13 +56,13 @@ type Task = () => Promise<void>;
 // An executable command.
 type Command = {
   task: Task,
-  command: Commands.Task,
+  command: Commands.Task | Commands.PersistentTask,
 } | {
   resolver: (value: void | PromiseLike<void>) => void,
   task: Task,
   command: Commands.TestTask,
 } | {
-  command: Exclude<Commands, Commands.Task | Commands.TestTask>,
+  command: Exclude<Commands, Commands.Task | Commands.TestTask | Commands.PersistentTask>,
 };
 
 /**
@@ -98,6 +107,19 @@ class Dispatcher {
     }
   }
 
+  /**
+   * Resolve all test resolvers.
+   *
+   * Used before clearing the queue in on a `Shutdown` or `Clear` command.
+   */
+  private unblockTestResolvers(): void {
+    this.queue.forEach(c => {
+      if (c.command === Commands.TestTask) {
+        c.resolver();
+      }
+    });
+  }
+
   /**
    * Executes all the commands in the queue, from oldest to newest.
    */
@@ -109,27 +131,23 @@ class Dispatcher {
         this.state = DispatcherState.Stopped;
         return;
       case(Commands.Shutdown):
-      case(Commands.Clear):
-        // Unblock test resolvers before clearing the queue.
-        this.queue.forEach(c => {
-          if (c.command === Commands.TestTask) {
-            c.resolver();
-          }
-        });
-
+        this.unblockTestResolvers();
         this.queue = [];
-        if (nextCommand.command === Commands.Clear) {
-          this.state = DispatcherState.Stopped;
-        } else {
-          this.state = DispatcherState.Shutdown;
-        }
-
+        this.state = DispatcherState.Shutdown;
         return;
+      case(Commands.Clear):
+        this.unblockTestResolvers();
+        this.queue = this.queue.filter(c =>
+          [Commands.PersistentTask, Commands.Shutdown].includes(c.command)
+        );
+        nextCommand = this.getNextCommand();
+        continue;
       case (Commands.TestTask):
         await this.executeTask(nextCommand.task);
         nextCommand.resolver();
         nextCommand = this.getNextCommand();
         continue;
+      case(Commands.PersistentTask):
       case(Commands.Task):
         await this.executeTask(nextCommand.task);
         nextCommand = this.getNextCommand();
@@ -237,6 +255,19 @@ class Dispatcher {
     });
   }
 
+  /**
+   * Works exactly like {@link launch},
+   * but enqueues a persistent task which is not cleared by the Clear command.
+   *
+   * @param task The task to enqueue.
+   */
+  launchPersistent(task: Task): void {
+    this.launchInternal({
+      task,
+      command: Commands.PersistentTask
+    });
+  }
+
   /**
    * Flushes the tasks enqueued while the dispatcher was uninitialized.
    *
@@ -268,9 +299,8 @@ class Dispatcher {
   /**
    * Enqueues a Clear command at the front of the queue and triggers execution.
    *
-   * The Clear command will remove all other tasks from the queue
-   * and put the dispatcher in a Stopped state after the command is executed.
-   * In order to re-start the dispatcher, call the `resume` method.
+   * The Clear command will remove all other tasks
+   * except for persistent tasks or shutdown tasks.
    *
    * # Note
    *
@@ -311,7 +341,7 @@ class Dispatcher {
    * Shutsdown the dispatcher.
    *
    * 1. Executes all tasks launched prior to this one.
-   * 2. Clears the queue of any tasks launched after this one.
+   * 2. Clears the queue of any tasks launched after this one (even persistent tasks).
    * 3. Puts the dispatcher in the `Shutdown` state.
    *
    * # Note
@@ -356,9 +386,10 @@ class Dispatcher {
       return;
     }
 
+    // Clear queue.
     this.clear();
-    // We need to wait for the clear command to be executed.
-    await this.testBlockOnQueue();
+    // Wait for the clear command and any persistent tasks that may still be in the queue.
+    await this.shutdown();
     this.state = DispatcherState.Uninitialized;
   }
 

glean/src/core/dispatcher.ts

@@ -114,8 +114,10 @@ class Glean {
    * This function is only supposed to be called when telemetry is disabled.
    */
   private static async clearMetrics(): Promise<void> {
-    // Stop ongoing upload jobs and clear pending pings queue.
-    Glean.pingUploader.clearPendingPingsQueue();
+    // Clear enqueued upload jobs and clear pending pings queue.
+    //
+    // The only job that will still be sent is the deletion-request ping.
+    await Glean.pingUploader.clearPendingPingsQueue();
 
     // There is only one metric that we want to survive after clearing all
     // metrics: first_run_date. Here, we store its value
@@ -216,7 +218,11 @@ class Glean {
     Context.pingsDatabase = new PingsDatabase(Glean.platform.Storage);
     Context.errorManager = new ErrorManager();
 
-    Glean.instance._pingUploader = new PingUploader(correctConfig, Glean.platform, Context.pingsDatabase);
+    Glean.instance._pingUploader = new PingUploader(
+      correctConfig,
+      Glean.platform,
+      Context.pingsDatabase
+    );
 
     Context.pingsDatabase.attachObserver(Glean.pingUploader);
 
@@ -504,17 +510,14 @@ class Glean {
     // Get back to an uninitialized state.
     await Context.testUninitialize();
 
+    // Shutdown the current uploader.
+    //
+    // This is fine because a new uploader is created on initialize.
+    // It will also guarantee all pings to be sent before uninitializing.
+    await Glean.pingUploader?.shutdown();
+
     // Deregister all plugins
     testResetEvents();
-
-    // Await ongoing jobs and clear pending pings queue.
-    if (Glean.pingUploader) {
-      await Glean.pingUploader.testBlockOnPingsQueue();
-
-      // The first time tests run, before Glean is initialized, we are
-      // not guaranteed to have an uploader. Account for this.
-      Glean.pingUploader.clearPendingPingsQueue();
-    }
   }
 
   /**