Merge pull request #1350 from mdboom/fix-python-subprocess

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v33.5.0...main)
 
+* Python
+  * BUGFIX: Network slowness or errors will no longer block the main dispatcher thread, leaving work undone on shutdown.
+
 # v33.5.0 (2020-12-01)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v33.4.0...v33.5.0)

glean-core/python/glean/_process_dispatcher.py

@@ -94,9 +94,10 @@ def dispatch(cls, func, args) -> Union[_SyncWorkWrapper, subprocess.Popen]:
 
         if Glean._configuration._allow_multiprocessing:
             # We only want one of these processes running at a time, so if
-            # there's already one, join on it. Therefore, this should not be
-            # run from the main user thread.
-            cls._wait_for_last_process()
+            # there's already one running, just bail out. It will pick up any
+            # newly-written pings as it processes the directory.
+            if cls._last_process is not None and cls._last_process.poll() is None:
+                return cls._last_process
 
             # This sends the data over as a commandline argument, which has a
             # maximum length of:

glean-core/python/tests/test_network.py

@@ -145,23 +145,31 @@ def test_ping_upload_worker_single_process(safe_httpserver):
     pings_dir = Glean._data_dir / "pending_pings"
     pings_dir.mkdir()
 
-    for _ in range(10):
+    for _ in range(5):
         with (pings_dir / str(uuid.uuid4())).open("wb") as fd:
             fd.write(b"/data/path/\n")
             fd.write(b"{}\n")
 
-    # Fire off two PingUploadWorker processing tasks at the same time. If
-    # working correctly, p1 should finish entirely before p2 starts.
-    # If these actually run in parallel, one or the other will try to send
-    # deleted queued ping files and fail.
+    # Fire off the first PingUploaderWorker process to handle the existing
+    # pings. Then, in parallel, write more pings and fire off "new"
+    # PingUploadWorker processes (some of which will be no-ops and just keeping
+    # the existing worker running).
     p1 = PingUploadWorker._process()
-    p2 = PingUploadWorker._process()
+
+    processes = []
+    for _ in range(5):
+        with (pings_dir / str(uuid.uuid4())).open("wb") as fd:
+            fd.write(b"/data/path/\n")
+            fd.write(b"{}\n")
+
+        processes.append(PingUploadWorker._process())
 
     p1.wait()
     assert p1.returncode == 0
 
-    p2.wait()
-    assert p2.returncode == 0
+    for p in processes:
+        p.wait()
+        assert p.returncode == 0
 
     assert 10 == len(safe_httpserver.requests)