Clarify when a DAP should respond to requests like terminateRequest... immediately, or when the request has been handled?

[DanTup]

I've always assumed that if the DAP client issues a request like terminate, that the DAP server should try to perform the termination before responding to this request (since this allows it to communicate a failure, and if the response was just always sent immediately before any work is done, it seems like it's essentially an event and doesn't need request/response).

However, I've noticed that if I don't respond to a terminate request within 2s, the VS Code client shows an error message that the termination timed out (even though it didn't, and the DAP server is still working on it). This happens a lot in test runs, because the Dart test runner will by default wait to allow the current test to complete when first trying to stop (to provide more consistency for teardowns, not having to have them deal with tests that were aborted mid-running).

I can change how this works to respond immediately, but I can't find anything in the docs that describe that this is how servers should behave, and it seems like something that should be (since if clients and servers interpret this differently - as has happened here - users may see confusing timeout messages).

[puremourning]

Seems like the client just has an overly aggressive timeout. I agree with your interpretation- client should wait for response. 2s is (sadly) not very long in debugger timescales in 2023

[DanTup]

Yeah, I generally think timeouts (particular short ones!) are a bad idea anyway. Give the user a progress notification and a cancel option and let them decide when to give up. The number of times I've known something would take a long time and I'm happy to go and make a cup of tea while it does it, but some arbitrary timeout aborts it when it's nearly done 🤷‍♂️

I'll hold off making changes to my DAP for now though in the hope VS Code can be updated to not be so aggressive.

[connor4312]

I agree that the preferred option here is a tweak to VS Code behavior. The response should be sent once termination completes.

[roblourens]

We have this timeout for terminate and disconnect, but I think the error dialog is only shown for 'terminate'. That might not be intentional, maybe we just missed the try/catch on terminate.

Would you want to say that on terminate/the first click of the stop button, we wait as long as possible to stop gracefully, but on disconnect/the second click, we will force kill the adapter if it doesn't reply after 2s?

[DanTup]

Would you want to say that on terminate/the first click of the stop button, we wait as long as possible to stop gracefully, but on disconnect/the second click, we will force kill the adapter if it doesn't reply after 2s?

I would rather it waited a little longer than 2s, since the DA might be trying its best to clean up (it may have spawned multiple processes, and might still want to try a graceful shutdown to avoid orphaned sub-processes before resorting to just killing the processes).

Although, my main concern here is with terminate (the graceful shutdown). VS Code is currently showing an error after 2s, but the DA may take much longer than this to shut down gracefully. In my case, the Dart test package will wait for the current test to complete, and that could take any amount of time - but it clearly prints that it's doing this in the console, and that the user can stop again to quit immediately. I would expect VS Code to wait indefinitely on terminate and not show any error/timeout.

[roblourens]

I think we agree, I'll get rid of the timeout on terminate. But if the user keeps clicking the shutdown button, and we force a shutdown, then orphaned processes are a risk.

[DanTup]

But if the user keeps clicking the shutdown button, and we force a shutdown, then orphaned processes are a risk.

Yep, I understand. I think the behaviour is fair, but perhaps the UX for this could be better - I've had complaints about orphaned processes that turned out to be users clicking the button a second time because it didn't seem like the first click worked and it wasn't clear that a second click would just terminate.

Perhaps the DA should be given some minimum time to be able to respond to Terminate (for ex. 1-2 seconds) where the Stop button is disabled, and if the session still hasn't terminated, it's re-enabled for Disconnect and the tooltip updated?

(FWIW, I find Terminate/Disconnect naming really confusing, and feel every time I'm debugging something I have to look it up to make sure I'm interpreting them the right way around 🙃)

[roblourens]

Perhaps the DA should be given some minimum time to be able to respond to Terminate (for ex. 1-2 seconds) where the Stop button is disabled, and if the session still hasn't terminated, it's re-enabled for Disconnect and the tooltip updated?

Would be great if we could show progress somehow

FWIW, I find Terminate/Disconnect naming really confusing

Same 😕

[DanTup]

Would be great if we could show progress somehow

I actually did this in the legacy Dart DAP:

https://github.com/Dart-Code/Dart-Code/blob/61cb1c799ae51845bb9f64f55316ece3b210243c/src/debug/dart_debug_impl.ts#L835

I didn't reproduce it in the new DAPs yet, perhaps VS Code could do it instead :-)

[jonahgraham]

@roblourens we are being affected by this behaviour and want to provide a PR for VSCode to resolve the situation. I see you added the March 2023 milestone to it. Is this something you have started working on, or would you be open to receiving a PR on it from us?

[jonahgraham]

Perhaps if I am specific that will help, we want to make it possible to have quite long (> 2 seconds for sure) time to complete a terminate/disconnect (we currently don't implement terminate request, just disconnect in the cdt-gdb-adapter).

The idea of having a progress monitor appear after 2 seconds to force the termination seems like a good way to resolve this as @DanTup mentioned.

So the PR will add such an implementation and remove the forced shutdown entirely.

It could be that some adapters today are "relying" on the existing behaviour, but I hope that we can expect those adapters to behave better rather than making this new functional optional.

[jonahgraham]

@roblourens gentle ping on this - we want to contribute to this fix, but want to make sure you are open in principle to such a fix before starting the work.

[roblourens]

Sorry, have been distracted by other tasks. I think we just want to remove the timeout on terminate for now, right? Pushed a PR for that. I think the scenario could use some UX thinking though. Seems that there should be some kind of feedback that something is happening when you wait- a form of the stop button that shows progress, disabling the stop button for a time, a progress notification that pops up after a few seconds?

[jonahgraham]

Thanks @roblourens - PR #178714 resolves the most critical part as it allows our adapter time to terminate cleanly.

Seems that there should be some kind of feedback that something is happening when you wait- a form of the stop button that shows progress, disabling the stop button for a time, a progress notification that pops up after a few seconds?

I agree - next is adding some UI to discourage users from pressing stop again while the current terminate is underway.

Disabling the stop button once it is pressed would be ideal, with a progress notification that pops up after 2 seconds with a button to force the terminate (by issuing the disconnectRequest) would be great.

Should we go further and provide something in the DAP to track progress of the terminate? At the moment that feels a little heavier than needed.

[DanTup]

Thanks! That timeout was the bit causing me grief too.

I agree with the above, although I'd suggest showing some progress indicator after less than 2s, because I think 2s with no visible update on screen is still long enough that a user might think "my click didn't work" and click again.

[jonahgraham]

What if the stop button is disabled on the first click? In that case is 2s too long? How long do you recommend?

TBH I was wondering if either the 2 seconds and/or the message displayed should be configurable by the extension author. In our case we know that the debug adapter takes about 3 seconds to shutdown and if the user interrupts that sequence the target hardware can be left in an inconsistent state

[DanTup]

What if the stop button is disabled on the first click? In that case is 2s too long? How long do you recommend?

If the button is disabled then I don't think waiting a little longer is too bad.

I don't know how useful configuring the delay before showing progress is though, I don't think it taking more or less time changes that it's useful to show the user that the termination is in progress. In our old debug adapter, we showed a progress immediately when you clicked the button and if termination was fast, it'd just disappear quickly.

[roblourens]

I opened #178735 for the UI, and I will close this for removing the timeout.