Repository: beeware/briefcase
Pull request #1541 (2023-11-15 15:09:45)

Wait for app to exit normally after log streaming
## Changes
- Ensure the app has exited once it has indicated an exit condition in its output while log streaming
- Also, closes the log streaming `Popen` process after log streaming and uses `poll()` to determine the returncode

## Notes
- This doesn't affect app log streaming using `--test` because Briefcase doesn't care what happens to the app after the exit condition is found in its output.
- However, when an app includes the exit condition in its output while not under testing, then Briefcase uses the returncode of the app's `Popen` process to determine success.
- In general, it does seem better to wait for the app to exit on its own...since that should be what it's doing anyway
- Alternatively, we could change log streaming to _always_ use the returncode in the output exit condition to determine success....I think I'm mostly ambivalent on this...

## PR Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
- [X] All new features have been tested
- [X] All new features have been documented
- [X] I have read the **CONTRIBUTING.md** file
- [X] I will abide by the code of conduct


Patch:
diff --git a/changes/1541.misc.rst b/changes/1541.misc.rst
new file mode 100644
index 00000000..6043dbdc
--- /dev/null
+++ b/changes/1541.misc.rst
@@ -0,0 +1,1 @@
+Once the exit condition is detected while log streaming, the app is now waited upon to exit normally instead of signaling it to exit immediately.
diff --git a/src/briefcase/commands/run.py b/src/briefcase/commands/run.py
index 6a54b499..4977b341 100644
--- a/src/briefcase/commands/run.py
+++ b/src/briefcase/commands/run.py
@@ -1,7 +1,9 @@
 from __future__ import annotations
 
 import re
+import subprocess
 from abc import abstractmethod
+from contextlib import suppress
 
 from briefcase.config import AppConfig
 from briefcase.exceptions import BriefcaseCommandError, BriefcaseTestSuiteFailure
@@ -79,6 +81,14 @@ class LogFilter:
             if self.exit_filter:
                 self.returncode = self.exit_filter(tail)
                 if self.returncode is not None:
+                    # This returncode is captured from output from the app and does not
+                    # necessarily mean the app has already exited. However, once
+                    # StopStreaming is raised, the output streamer sends a signal to
+                    # the app to exit immediately and that may result in the app
+                    # exiting with a non-zero returncode. Therefore, wait for the app
+                    # to close normally before raising StopStreaming.
+                    with suppress(subprocess.TimeoutExpired):
+                        self.log_popen.wait(timeout=3)
                     raise StopStreaming()
 
         # Return the display line
@@ -131,7 +141,8 @@ class RunAppMixin:
         Catches and cleans up after any Ctrl-C interrupts.
 
         :param app: The app to be launched
-        :param popen: The Popen object for the stream we are monitoring
+        :param popen: The Popen object for the stream we are monitoring; this Popen
+            process will be closed after log streaming completes.
         :param test_mode: Are we launching in test mode?
         :param clean_filter: The log cleaning filter to use; see ``LogFilter``
             for details.
@@ -158,12 +169,13 @@ class RunAppMixin:
 
             # Start streaming logs for the app.
             self.logger.info("=" * 75)
-            self.tools.subprocess.stream_output(
-                label="log stream" if log_stream else app.app_name,
-                popen_process=popen,
-                stop_func=stop_func,
-                filter_func=log_filter,
-            )
+            with popen:
+                self.tools.subprocess.stream_output(
+                    label="log stream" if log_stream else app.app_name,
+                    popen_process=popen,
+                    stop_func=stop_func,
+                    filter_func=log_filter,
+                )
 
             # If we're in test mode, and log streaming ends,
             # check for the status of the test suite.
@@ -186,7 +198,7 @@ class RunAppMixin:
             else:
                 # If we're monitoring an actual app (not just a log stream),
                 # and the app didn't exit cleanly, surface the error to the user.
-                if popen.returncode != 0:
+                if popen.poll() != 0:
                     raise BriefcaseCommandError(f"Problem running app {app.app_name}.")
 
         except KeyboardInterrupt:


Test patch:
diff --git a/tests/commands/run/test__stream_app_logs.py b/tests/commands/run/test__stream_app_logs.py
index d8b6188c..74c1287b 100644
--- a/tests/commands/run/test__stream_app_logs.py
+++ b/tests/commands/run/test__stream_app_logs.py
@@ -9,7 +9,7 @@ from briefcase.exceptions import BriefcaseCommandError, BriefcaseTestSuiteFailur
 def test_run_app(run_command, first_app):
     """An app can have its logs streamed."""
     popen = mock.MagicMock()
-    popen.returncode = 0
+    popen.poll = mock.MagicMock(return_value=0)
     clean_filter = mock.MagicMock()
 
     # Invoke the stop func as part of streaming. This is to satisfy coverage.
@@ -47,7 +47,7 @@ def test_run_app(run_command, first_app):
 def test_run_app_custom_stop_func(run_command, first_app):
     """An app with a custom stop function can have its logs streamed."""
     popen = mock.MagicMock()
-    popen.returncode = 0
+    popen.poll = mock.MagicMock(return_value=0)
     clean_filter = mock.MagicMock()
     stop_func = mock.MagicMock()
     run_command.tools.subprocess.stream_output = mock.MagicMock()