Fix DAP defer_stop_events implementation

DAP requests have a "defer_stop_events" option that is intended to
defer the emission of any "stopped" event until after the current
request completes.  This was needed to handle async continues like
"finish &".

However, I noticed that sometimes DAP tests can fail, because a stop
event does arrive before the response to the "stepOut" request.  I've
only noticed this when the machine is fairly loaded -- for instance
when I'm regression-testing a series, it may occur in some of the
tests mid-series.

I believe the problem is that the implementation in the "request"
function is incorrect -- the flag is set when "request" is invoked,
but instead it must be deferred until the request itself is run.  That
is, the setting must be captured in one of the wrapper functions.

Following up on this, Simon pointed out that introducing a delay
before sending a request's response will cause test case failures.
That is, there's a race here that is normally hidden.

Investigation showed that that deferred requests can't force event
deferral.  This patch implements this; but more testing showed many
more race failures.  Some of these are due to how the test suite is
written.

Anyway, in the end I took the radical approach of deferring all events
by default.  Most DAP requests are asynchronous by nature, so this
seemed ok.  The only case I found that really required this is
pause.exp, where the test (rightly) expects to see a 'continued' event
while performing an inferior function call.

I went through all events and all requests and tried to convince
myself that this patch will cause acceptable behavior in every case.
However, it's hard to be completely sure about this approach.  Maybe
there are cases that do still need an event before the response, but
we just don't have tests for them.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32685
Acked-By: Simon Marchi <[email protected]>

Author: tromey

gdb/python/lib/gdb/dap/evaluate.py

@@ -69,7 +69,7 @@ def _repl(command, frame_id):
     }
 
 
-@request("evaluate")
+@request("evaluate", defer_events=False)
 @capability("supportsEvaluateForHovers")
 @capability("supportsValueFormattingOptions")
 def eval_request(
@@ -110,7 +110,7 @@ def variables(
 
 
 @capability("supportsSetExpression")
-@request("setExpression")
+@request("setExpression", defer_events=False)
 def set_expression(
     *, expression: str, value: str, frameId: Optional[int] = None, format=None, **args
 ):
@@ -126,7 +126,7 @@ def set_expression(
 
 
 @capability("supportsSetVariable")
-@request("setVariable")
+@request("setVariable", defer_events=False)
 def set_variable(
     *, variablesReference: int, name: str, value: str, format=None, **args
 ):

gdb/python/lib/gdb/dap/events.py

@@ -17,7 +17,7 @@
 
 from .modules import is_module, make_module
 from .scopes import set_finish_value
-from .server import send_event, send_event_maybe_later
+from .server import send_event
 from .startup import exec_and_log, in_gdb_thread, log
 
 # True when the inferior is thought to be running, False otherwise.
@@ -240,7 +240,7 @@ def _on_stop(event):
     else:
         obj["reason"] = stop_reason_map[event.details["reason"]]
     _expected_pause = False
-    send_event_maybe_later("stopped", obj)
+    send_event("stopped", obj)
 
 
 # This keeps a bit of state between the start of an inferior call and

gdb/python/lib/gdb/dap/next.py

@@ -16,7 +16,7 @@
 import gdb
 
 from .events import exec_and_expect_stop
-from .server import capability, request, send_gdb, send_gdb_with_response
+from .server import capability, request
 from .startup import in_gdb_thread
 from .state import set_thread
 
@@ -73,19 +73,14 @@ def step_in(
     exec_and_expect_stop(cmd)
 
 
-@request("stepOut", defer_stop_events=True)
+@request("stepOut")
 def step_out(*, threadId: int, singleThread: bool = False, **args):
     _handle_thread_step(threadId, singleThread, True)
     exec_and_expect_stop("finish &", propagate_exception=True)
 
 
-# This is a server-side request because it is funny: it wants to
-# 'continue' but also return a result, which precludes using
-# response=False.  Using 'continue &' would mostly work ok, but this
-# yields races when a stop occurs before the response is sent back to
-# the client.
-@request("continue", on_dap_thread=True)
+@request("continue")
 def continue_request(*, threadId: int, singleThread: bool = False, **args):
-    locked = send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread))
-    send_gdb(lambda: exec_and_expect_stop("continue"))
+    locked = _handle_thread_step(threadId, singleThread)
+    exec_and_expect_stop("continue &")
     return {"allThreadsContinued": not locked}

gdb/python/lib/gdb/dap/server.py

@@ -73,6 +73,13 @@ def set_request(self, req, result):
         self._req = req
         self._result = result
 
+    @in_dap_thread
+    def defer_events(self):
+        """Return True if events should be deferred during execution.
+
+        This may be overridden by subclasses."""
+        return True
+
     @in_dap_thread
     def invoke(self):
         """Implement the deferred request.
@@ -95,7 +102,10 @@ def reschedule(self):
 
         """
         with _server.canceller.current_request(self._req):
+            if self.defer_events():
+                _server.set_defer_events()
             _server.invoke_request(self._req, self._result, self.invoke)
+        _server.emit_pending_events()
 
 
 # A subclass of Exception that is used solely for reporting that a
@@ -230,7 +240,7 @@ def __init__(self, in_stream, out_stream, child_stream):
         self._out_stream = out_stream
         self._child_stream = child_stream
         self._delayed_fns_lock = threading.Lock()
-        self.defer_stop_events = False
+        self._defer_events = False
         self._delayed_fns = []
         # This queue accepts JSON objects that are then sent to the
         # DAP client.  Writing is done in a separate thread to avoid
@@ -316,7 +326,7 @@ def fn():
     def _read_inferior_output(self):
         while True:
             line = self._child_stream.readline()
-            self.send_event(
+            self.send_event_maybe_later(
                 "output",
                 {
                     "category": "stdout",
@@ -355,6 +365,17 @@ def _reader_thread(self):
         # When we hit EOF, signal it with None.
         self._read_queue.put(None)
 
+    @in_dap_thread
+    def emit_pending_events(self):
+        """Emit any pending events."""
+        fns = None
+        with self._delayed_fns_lock:
+            fns = self._delayed_fns
+            self._delayed_fns = []
+            self._defer_events = False
+        for fn in fns:
+            fn()
+
     @in_dap_thread
     def main_loop(self):
         """The main loop of the DAP server."""
@@ -371,13 +392,7 @@ def main_loop(self):
             req = cmd["seq"]
             with self.canceller.current_request(req):
                 self._handle_command(cmd)
-            fns = None
-            with self._delayed_fns_lock:
-                fns = self._delayed_fns
-                self._delayed_fns = []
-                self.defer_stop_events = False
-            for fn in fns:
-                fn()
+            self.emit_pending_events()
         # Got the terminate request.  This is handled by the
         # JSON-writing thread, so that we can ensure that all
         # responses are flushed to the client before exiting.
@@ -386,13 +401,13 @@ def main_loop(self):
         send_gdb("quit")
 
     @in_dap_thread
-    def send_event_later(self, event, body=None):
-        """Send a DAP event back to the client, but only after the
-        current request has completed."""
+    def set_defer_events(self):
+        """Defer any events until the current request has completed."""
         with self._delayed_fns_lock:
-            self._delayed_fns.append(lambda: self.send_event(event, body))
+            self._defer_events = True
 
-    @in_gdb_thread
+    # Note that this does not need to be run in any particular thread,
+    # because it uses locks for thread-safety.
     def send_event_maybe_later(self, event, body=None):
         """Send a DAP event back to the client, but if a request is in-flight
         within the dap thread and that request is configured to delay the event,
@@ -401,10 +416,10 @@ def send_event_maybe_later(self, event, body=None):
         with self.canceller.lock:
             if self.canceller.in_flight_dap_thread:
                 with self._delayed_fns_lock:
-                    if self.defer_stop_events:
-                        self._delayed_fns.append(lambda: self.send_event(event, body))
+                    if self._defer_events:
+                        self._delayed_fns.append(lambda: self._send_event(event, body))
                         return
-        self.send_event(event, body)
+        self._send_event(event, body)
 
     @in_dap_thread
     def call_function_later(self, fn):
@@ -415,7 +430,7 @@ def call_function_later(self, fn):
     # Note that this does not need to be run in any particular thread,
     # because it just creates an object and writes it to a thread-safe
     # queue.
-    def send_event(self, event, body=None):
+    def _send_event(self, event, body=None):
         """Send an event to the DAP client.
         EVENT is the name of the event, a string.
         BODY is the body of the event, an arbitrary object."""
@@ -439,14 +454,6 @@ def send_event(event, body=None):
     """Send an event to the DAP client.
     EVENT is the name of the event, a string.
     BODY is the body of the event, an arbitrary object."""
-    _server.send_event(event, body)
-
-
-def send_event_maybe_later(event, body=None):
-    """Send a DAP event back to the client, but if a request is in-flight
-    within the dap thread and that request is configured to delay the event,
-    wait until the response has been sent until the event is sent back to
-    the client."""
     _server.send_event_maybe_later(event, body)
 
 
@@ -476,7 +483,7 @@ def request(
     response: bool = True,
     on_dap_thread: bool = False,
     expect_stopped: bool = True,
-    defer_stop_events: bool = False
+    defer_events: bool = True
 ):
     """A decorator for DAP requests.
 
@@ -498,9 +505,9 @@ def request(
     inferior is running.  When EXPECT_STOPPED is False, the request
     will proceed regardless of the inferior's state.
 
-    If DEFER_STOP_EVENTS is True, then make sure any stop events sent
-    during the request processing are not sent to the client until the
-    response has been sent.
+    If DEFER_EVENTS is True, then make sure any events sent during the
+    request processing are not sent to the client until the response
+    has been sent.
     """
 
     # Validate the parameters.
@@ -523,26 +530,33 @@ def wrap(func):
 
         # Verify that the function is run on the correct thread.
         if on_dap_thread:
-            cmd = in_dap_thread(func)
+            check_cmd = in_dap_thread(func)
         else:
             func = in_gdb_thread(func)
 
             if response:
-                if defer_stop_events:
-                    if _server is not None:
-                        with _server.delayed_events_lock:
-                            _server.defer_stop_events = True
 
                 def sync_call(**args):
                     return send_gdb_with_response(lambda: func(**args))
 
-                cmd = sync_call
+                check_cmd = sync_call
             else:
 
                 def non_sync_call(**args):
                     return send_gdb(lambda: func(**args))
 
-                cmd = non_sync_call
+                check_cmd = non_sync_call
+
+        if defer_events:
+
+            def deferring(**args):
+                _server.set_defer_events()
+                return check_cmd(**args)
+
+            cmd = deferring
+
+        else:
+            cmd = check_cmd
 
         # If needed, check that the inferior is not running.  This
         # wrapping is done last, so the check is done first, before
@@ -582,7 +596,7 @@ def client_bool_capability(name, default=False):
 @request("initialize", on_dap_thread=True)
 def initialize(**args):
     _server.config = args
-    _server.send_event_later("initialized")
+    _server.send_event_maybe_later("initialized")
     global _lines_start_at_1
     _lines_start_at_1 = client_bool_capability("linesStartAt1", True)
     global _columns_start_at_1

gdb/testsuite/gdb.dap/attach.exp

@@ -33,11 +33,11 @@ set attach_id [dap_attach $testpid $binfile]
 
 dap_check_request_and_response "configurationDone" configurationDone
 
+dap_check_response "attach response" attach $attach_id
+
 dap_wait_for_event_and_check "stopped" stopped \
     "body reason" attach
 
-dap_check_response "attach response" attach $attach_id
-
 dap_shutdown true
 
 kill_wait_spawned_process $test_spawn_id