fix: check if thread is still paused before handling stacktrace command result

[sidlatau]

Fixes #1363

[mfussenegger]

I'm a bit confused about what kind of error behavior you're seeing. In the other issue you mention:

Sometimes it stops at some random spot in framework code, sometimes it shows error "Thread 1 is not paused".

Seems to me that this would only protect against the second scenario and that could probably be changed to:

diff --git a/lua/dap/session.lua b/lua/dap/session.lua
index 2bb16f5..435411b 100644
--- a/lua/dap/session.lua
+++ b/lua/dap/session.lua
@@ -744,11 +744,15 @@ function Session:event_stopped(stopped)
       startFrame = 0,
       threadId = stopped.threadId
     }
     local err, response = self:request('stackTrace', params)
     if err then
-      utils.notify('Error retrieving stack traces: ' .. utils.fmt_error(err), vim.log.levels.ERROR)
+      if thread.stopped then
+        utils.notify('Error retrieving stack traces: ' .. utils.fmt_error(err), vim.log.levels.ERROR)
+      else
+        log.debug("Debug adapter resumed during stopped event handling", thread, err)
+      end
       return
     end
     local frames = response.stackFrames --[=[@as dap.StackFrame[]]=]
     thread.frames = frames
     local current_frame = get_top_frame(frames)

But I'm not sure how to fix the stopping at random-spot aspect.
Seems like in that case the stacktrace goes through, but the debug-adapter continues after? If that's the case, I don't see how that can be fixed on the client side at all - it's an inherent race.

[sidlatau]

I see in the suggestion that thread.stopped is checked only in the error block.
But there could be such situation:
Thread is stopped, stacktrace command is called, it returns response, not an error, but thread is not stopped anymore. In this situation DAP client tries to stop at location provided in stacktrace command result. That is what I meant by saying "Sometimes it stops at some random spot in framework code". I think thread.stopped check should be done not only in error block, but before it.

:mess                                                                                                                         
Before stackTrace ----------{                                                                                                 
  id = 1,                                                                                                                     
  name = "main",                                                                                                              
  stopped = true                                                                                                              
}                                                                                                                             
After stackTrace ----------{                                                                                                  
  id = 1,                                                                                                                     
  name = "main",                                                                                                              
  stopped = false                                                                                                             
}                                                                                                                             
StackTrace err----------nil                                                                                                   
StackTrace response---------{                                                                                                 
  stackFrames = { {                                                                                                           
      canRestart = false,                                                                                                     
      column = 1,                                                                                                             
      id = 1,                                                                                                                 
      line = 1397,                                                                                                            
      name = "runApp",                                                                                                        
      source = {                                                                                                              
        adapterData = {                                                                                                       
          fixedId = true,                                                                                                     
          id = "libraries/@237005770/scripts/package%3Aflutter%2Fsrc%2Fwidgets%2Fbinding.dart/1932f594b60",                   
          type = "@Script",                                                                                                   
          uri = "package:flutter/src/widgets/binding.dart"                                                                    
        },                                                                                                                    
        name = "package:flutter/src/widgets/binding.dart",                                                                    
        origin = "from external packages",                                                                                    
        path = "/Users/ts/fvm/versions/3.24.5/packages/flutter/lib/src/widgets/binding.dart",                                 
        presentationHint = "deemphasize"                                                                                      
      }                                                                                                                       
    }, {                                                                                                                      
      canRestart = true,                                                                                                      
      column = 3,                                                                                                             
      id = 2,                                                                                                                 
      line = 4,                                                                                                               
      name = "main",                                                                                                          
      source = {                                                                                                              
        adapterData = {                                                                                                       
          fixedId = true,                                                                                                     
          id = "libraries/@17089780/scripts/package%3Atest%2Fmain.dart/1932f594b59",                                          
          type = "@Script",                                                                                                   
          uri = "package:test/main.dart"                                                                                      
        },                                                                                                                    
        name = "package:test/main.dart",                                                                                      
-- More --                        

[sidlatau]

For "Thread 1 is not paused" error, here is comment dart-lang/sdk#57006 (comment) from @DanTup

Therefore, my expectation is that DAP clients will gracefully handle failures to requests like stackTrace to account for these kinds of races. As far as I can tell, there is no standard (eg. an error code) for this in DAP though - so if that would be useful, we could perhaps file a DAP issue to add this and provide some guidance for this kind of situation.

Maybe a standard error code is a way to handle it? The best solution would be to not show such an error, as it is not actionable.

[DanTup]

I think thread.stopped check should be done not only in error block, but before it.

I'm unfamiliar with this code, but this logic sounds right to me. It's possible the stackTrace request succeeds, but there was a continue event around the same time. The editor shouldn't continue to jump to the location at the stop of the stack if the thread has since resumed, even though stackTrace did not fail (because of timing).

That said, I don't know if checking thread.stopped is sound - what if the thread stopped again immediately after (eg. it hit a breakpoint), could this be true even though the current stackTrace response being processed is now stale? Should the check really be "is the thread still stopped by the same event that triggered this stackTrace request"?

[mfussenegger]

It's possible the stackTrace request succeeds, but there was a continue event around the same time. The editor shouldn't continue to jump to the location at the stop of the stack if the thread has since resumed, even though stackTrace did not fail (because of timing).

My concern is that even if it checks the stopped state again on a successful stacktrace response there's still a race if the debug adapter doesn't do any synchronization.

If I understand it right, the debug adapter basically does:

• send stopped
• send continued

nvim-dap is mostly single-threaded, with event loop based cooperative concurrency, so there are the following ways it can observe and react to the events:

• receives stopped
• asks for stack trace
• receives continued
• receives stack trace; realized it got continued and stops

Or:

• receives stopped
• receives continued
• (given it is busy processing stopped, it won't process continued until going async)
• asks for stacktrace
• processes continued
• receives stacktrace; realized it got continued and stops

With this PR these scenarios would be fine, but there's also:

• receives stopped
• asks for stacktrace
• receives stacktrace
• jumps to first frame
• receives continued

I also don't see how the client could handle ^ even if it weren't for the threading model, at some point it must act on a given state and jump to the frame. It can't assume that there might be coming a debug adapter generated continue any second.

(Note the PR here is fine for me, but if I understand the scenario right there's still a potential race left)

[DanTup]

With this PR these scenarios would be fine, but there's also:

receives stopped
asks for stacktrace
receives stacktrace
jumps to first frame
receives continued

Yes, I think in this case you have to jump to the stack frame. However when the continued arrives, it should be clear that the thread continued (the editor should not continue to show being paused). This does mean the editor might jump to "random" code and then execution is not paused - but I think that's unavoidable given the behaviour being observed. I haven't been able to repro that in Dart yet though (we're still discussing at dart-lang/sdk#57006 (comment)), I see a difference in behaviour (empty call stack when paused-at-entry, and the continued event arriving immediately) but once I can, it's possible there are things we could tweak to reduce the chance of hitting this case.