[3.15] gh-145306: Fix browser open after empty export (GH-150017) (#152447)

gh-145306: Fix browser open after empty export (GH-150017)
(cherry picked from commit 860f8a5addcb556c251b8b9caa95d80518928067)

Co-authored-by: ivonastojanovic <80911834+ivonastojanovic@users.noreply.github.com>

diff --git a/Lib/profiling/sampling/binary_collector.py b/Lib/profiling/sampling/binary_collector.py
index 64afe632fae175fc85bc20847f484095ed5199bf..afbbc82926906780d4205aaae66ad64caf023d20 100644 (file)
--- a/Lib/profiling/sampling/binary_collector.py
+++ b/Lib/profiling/sampling/binary_collector.py
@@ -94,6 +94,7 @@ class BinaryCollector(Collector):
             filename: Ignored (binary files are written incrementally)
         """
         self._writer.finalize()
+        return True
 
     @property
     def total_samples(self):

diff --git a/Lib/profiling/sampling/cli.py b/Lib/profiling/sampling/cli.py
index 0330c15c014545ceef2f912a824c4bf867136098..466b0aceae2dcc6affb475a316cd0f4eb403f0d3 100644 (file)
--- a/Lib/profiling/sampling/cli.py
+++ b/Lib/profiling/sampling/cli.py
@@ -117,6 +117,9 @@ COLLECTOR_MAP = {
     "binary": BinaryCollector,
 }
 
+BROWSER_COMPATIBLE_FORMATS = ("flamegraph", "diff_flamegraph", "heatmap")
+
+
 def _setup_child_monitor(args, parent_pid):
     # Build CLI args for child profilers (excluding --subprocesses to avoid recursion)
     child_cli_args = _build_child_profiler_args(args)
@@ -528,8 +531,12 @@ def _add_format_options(parser, include_compression=True, include_binary=True):
     output_group.add_argument(
         "--browser",
         action="store_true",
-        help="Automatically open HTML output (flamegraph, heatmap) in browser. "
-        "When using `--subprocesses`, only the main process opens the browser",
+        help=(
+            "Automatically open HTML output "
+            f"({', '.join('--' + f.replace('_', '-') for f in BROWSER_COMPATIBLE_FORMATS)}) "
+            "in browser. "
+            "When using `--subprocesses`, only the main process opens the browser"
+        ),
     )
 
 
@@ -789,13 +796,12 @@ def _replay_with_reader(args, reader):
             args.outfile
             or _generate_output_filename(args.format, os.getpid())
         )
-        collector.export(filename)
+        export_ok = collector.export(filename)
 
         # Auto-open browser for HTML output if --browser flag is set
         if (
-            args.format in (
-                'flamegraph', 'diff_flamegraph', 'heatmap'
-            )
+            export_ok
+            and args.format in BROWSER_COMPATIBLE_FORMATS
             and getattr(args, 'browser', False)
         ):
             _open_in_browser(filename)
@@ -840,10 +846,14 @@ def _handle_output(collector, args, pid, mode):
             filename = os.path.join(args.outfile, _generate_output_filename(args.format, pid))
         else:
             filename = args.outfile or _generate_output_filename(args.format, pid)
-        collector.export(filename)
+        export_ok = collector.export(filename)
 
         # Auto-open browser for HTML output if --browser flag is set
-        if args.format in ('flamegraph', 'diff_flamegraph', 'heatmap') and getattr(args, 'browser', False):
+        if (
+            export_ok
+            and args.format in BROWSER_COMPATIBLE_FORMATS
+            and getattr(args, 'browser', False)
+        ):
             _open_in_browser(filename)
 
 

diff --git a/Lib/profiling/sampling/collector.py b/Lib/profiling/sampling/collector.py
index 8e0f0c44c4f8f367e7751bcba7990e5475dd3d87..1dc3656e0ebe97692d53938d986f288a3f362d76 100644 (file)
--- a/Lib/profiling/sampling/collector.py
+++ b/Lib/profiling/sampling/collector.py
@@ -163,7 +163,11 @@ class Collector(ABC):
 
     @abstractmethod
     def export(self, filename):
-        """Export collected data to a file."""
+        """Export collected data.
+
+        Returns:
+            bool: True if output was generated, False if there was no data to export.
+        """
 
     @staticmethod
     def _filter_internal_frames(frames):

diff --git a/Lib/profiling/sampling/gecko_collector.py b/Lib/profiling/sampling/gecko_collector.py
index 361f6037f216fdc7444ce763299aad1c9cd9514d..2bb5bd2f664d59fc7216a19d841a748c7db4cfa0 100644 (file)
--- a/Lib/profiling/sampling/gecko_collector.py
+++ b/Lib/profiling/sampling/gecko_collector.py
@@ -756,6 +756,7 @@ class GeckoCollector(Collector):
         print(
             f"Open in Firefox Profiler: https://profiler.firefox.com/"
         )
+        return True
 
     def _build_marker_schema(self):
         """Build marker schema definitions for Firefox Profiler."""

diff --git a/Lib/profiling/sampling/heatmap_collector.py b/Lib/profiling/sampling/heatmap_collector.py
index 6e650ec08f410bc76ae844a11f5c6cc231b2593d..0361fd357b222ddde87720b0d6885c69ce464aa2 100644 (file)
--- a/Lib/profiling/sampling/heatmap_collector.py
+++ b/Lib/profiling/sampling/heatmap_collector.py
@@ -598,7 +598,7 @@ class HeatmapCollector(StackTraceCollector):
         """
         if not self.file_samples:
             print("Warning: No heatmap data to export")
-            return
+            return False
 
         try:
             output_dir = self._prepare_output_directory(output_path)
@@ -612,6 +612,7 @@ class HeatmapCollector(StackTraceCollector):
             self._generate_index_html(output_dir / 'index.html', file_stats)
 
             self._print_export_summary(output_dir, file_stats)
+            return True
 
         except Exception as e:
             print(f"Error: Failed to export heatmap: {e}")

diff --git a/Lib/profiling/sampling/jsonl_collector.py b/Lib/profiling/sampling/jsonl_collector.py
index 5aa42ef09024dc303ba50f84d44d04a2885ce6ba..41b0456d6f56d3c230f72c2bc0d9a9307f092e8c 100644 (file)
--- a/Lib/profiling/sampling/jsonl_collector.py
+++ b/Lib/profiling/sampling/jsonl_collector.py
@@ -165,6 +165,7 @@ class JsonlCollector(StackTraceCollector):
             )
             self._write_message(output, self._build_end_record())
         print(f"JSONL profile written to {filename}")
+        return True
 
     def _build_meta_record(self):
         record = {

diff --git a/Lib/profiling/sampling/pstats_collector.py b/Lib/profiling/sampling/pstats_collector.py
index 43b1daf2a119d4ea7777bc5b4266343b256fb867..7132cffd58f094a04e857116913b0d75c58a395c 100644 (file)
--- a/Lib/profiling/sampling/pstats_collector.py
+++ b/Lib/profiling/sampling/pstats_collector.py
@@ -63,6 +63,7 @@ class PstatsCollector(Collector):
     def export(self, filename):
         self.create_stats()
         self._dump_stats(filename)
+        return True
 
     def _dump_stats(self, file):
         stats_with_marker = dict(self.stats)

diff --git a/Lib/profiling/sampling/stack_collector.py b/Lib/profiling/sampling/stack_collector.py
index 42281dc6454c83c99dfc2a45c436c99d910da10d..eb1a3fba93cf33b62ce383dc8c917931ef72ccfc 100644 (file)
--- a/Lib/profiling/sampling/stack_collector.py
+++ b/Lib/profiling/sampling/stack_collector.py
@@ -64,6 +64,7 @@ class CollapsedStackCollector(StackTraceCollector):
             for stack, count in lines:
                 f.write(f"{stack} {count}\n")
         print(f"Collapsed stack output written to {filename}")
+        return True
 
 
 class FlamegraphCollector(StackTraceCollector):
@@ -161,7 +162,7 @@ class FlamegraphCollector(StackTraceCollector):
             print(
                 "Warning: No functions found in profiling data. Check if sampling captured any data."
             )
-            return
+            return False
 
         html_content = self._create_flamegraph_html(flamegraph_data)
 
@@ -169,6 +170,7 @@ class FlamegraphCollector(StackTraceCollector):
             f.write(html_content)
 
         print(f"Flamegraph saved to: {filename}")
+        return True
 
     @staticmethod
     @functools.lru_cache(maxsize=None)

diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_cli.py b/Lib/test/test_profiling/test_sampling_profiler/test_cli.py
index 0181095ca21e37580080dd489a06fa963bfd976b..3448258eca5d6c2120dcf72f8f89f8e71606d6a6 100644 (file)
--- a/Lib/test/test_profiling/test_sampling_profiler/test_cli.py
+++ b/Lib/test/test_profiling/test_sampling_profiler/test_cli.py
@@ -7,6 +7,7 @@ import subprocess
 import sys
 import tempfile
 import unittest
+from types import SimpleNamespace
 from unittest import mock
 
 try:
@@ -26,6 +27,7 @@ from profiling.sampling.cli import (
     FORMAT_EXTENSIONS,
     _create_collector,
     _generate_output_filename,
+    _handle_output,
     main,
 )
 from profiling.sampling.constants import (
@@ -727,6 +729,26 @@ class TestSampleProfilerCLI(unittest.TestCase):
             call_kwargs = mock_sample.call_args[1]
             self.assertEqual(call_kwargs.get("async_aware"), "running")
 
+    def test_handle_output_browser_not_opened_when_export_fails(self):
+        for format_type in ("flamegraph", "diff_flamegraph", "heatmap"):
+            with self.subTest(format=format_type):
+                collector = mock.MagicMock()
+                collector.export.return_value = False
+                args = SimpleNamespace(
+                    format=format_type,
+                    outfile="profile.html",
+                    browser=True,
+                )
+
+                with (
+                    mock.patch("profiling.sampling.cli.os.path.isdir", return_value=False),
+                    mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
+                ):
+                    _handle_output(collector, args, pid=12345, mode=0)
+
+                collector.export.assert_called_once_with("profile.html")
+                mock_open.assert_not_called()
+
     def test_async_aware_with_async_mode_all(self):
         """Test --async-aware with --async-mode all."""
         test_args = ["profiling.sampling.cli", "attach", "12345", "--async-aware", "--async-mode", "all"]

diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py b/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py
index 1ab31af67fec522c9ffdecf59216efffd87e03be..56f3fe5e1c2605c775b292206ae6da864ef302f6 100644 (file)
--- a/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py
+++ b/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py
@@ -539,9 +539,10 @@ class TestSampleProfilerComponents(unittest.TestCase):
 
         # Export flamegraph
         with captured_stdout(), captured_stderr():
-            collector.export(flamegraph_out.name)
+            export_ok = collector.export(flamegraph_out.name)
 
         # Verify file was created and contains valid data
+        self.assertTrue(export_ok)
         self.assertTrue(os.path.exists(flamegraph_out.name))
         self.assertGreater(os.path.getsize(flamegraph_out.name), 0)
 
@@ -560,6 +561,21 @@ class TestSampleProfilerComponents(unittest.TestCase):
         self.assertIn('"value":', content)
         self.assertIn('"children":', content)
 
+    def test_flamegraph_collector_empty_export_fails(self):
+        """Test empty flamegraph export reports no output."""
+        flamegraph_out = tempfile.NamedTemporaryFile(
+            suffix=".html", delete=False
+        )
+        self.addCleanup(close_and_unlink, flamegraph_out)
+
+        collector = FlamegraphCollector(1000)
+
+        with captured_stdout(), captured_stderr():
+            export_ok = collector.export(flamegraph_out.name)
+
+        self.assertFalse(export_ok)
+        self.assertEqual(os.path.getsize(flamegraph_out.name), 0)
+
     def test_gecko_collector_basic(self):
         """Test basic GeckoCollector functionality."""
         collector = GeckoCollector(1000)
@@ -1666,8 +1682,9 @@ class TestSampleProfilerComponents(unittest.TestCase):
         self.addCleanup(close_and_unlink, flamegraph_out)
 
         with captured_stdout(), captured_stderr():
-            diff.export(flamegraph_out.name)
+            export_ok = diff.export(flamegraph_out.name)
 
+        self.assertTrue(export_ok)
         self.assertTrue(os.path.exists(flamegraph_out.name))
         self.assertGreater(os.path.getsize(flamegraph_out.name), 0)