Breakpoints come back if removed during a hot restart

[DanTup]

This came up at Dart-Code/Dart-Code#3829, though I believe it's happening in dwds.

If you remove a breakpoint at the right point during a restart (it's a narrow window, but removing breakpoints after you just fixed something is probably not uncommon), it still is hit - but since your IDE doesn't know about it, you're unable to remove it and you continue to hit it until you restart your debugging session.

I believe it happens because during a restart, dwds moves all breakpoints to a set of "disabledBreakpoints", performs the restart, and then puts them back. If the IDE removed them during that period, I believe they remain in disabledBreakpoints and come back.

Here's an excerpt from the log:

# User triggers a reload by saving a file in the IDE

[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Attempting a hot restart
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Attempting to disable breakpoints and resume the isolate
[3:58:29 PM] [DAP] [Info] ==> {"seq":4853,"type":"event","event":"output","body":{"category":"stdout","output":"[   +6 ms] DwdsVmClient: Successfully disabled breakpoints and resumed the isolate\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4855,"type":"event","event":"output","body":{"category":"stdout","output":"[        ] DwdsVmClient: Attempting to get execution context ID.\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4857,"type":"event","event":"output","body":{"category":"stdout","output":"[        ] DwdsVmClient: Got execution context ID.\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4859,"type":"event","event":"output","body":{"category":"stdout","output":"[        ] DwdsVmClient: Issuing $dartHotRestartDwds request\n"}}
[3:58:29 PM] [VmService] [Info] [Flutter (Chrome)] <== {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointRemoved","timestamp":1646755109859,"isolate":{"type":"@Isolate","id":"6201","number":"6201","name":"main()","isSystemIsolate":false},"breakpoint":{"type":"Breakpoint","id":"bp/7110#170","breakpointNumber":7230,"enabled":true,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","id":"7110","uri":"package:gallery/studies/shrine/login.dart"},"tokenPos":88456}}}}}
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [   +6 ms] DwdsVmClient: Successfully disabled breakpoints and resumed the isolate
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Attempting to get execution context ID.
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Got execution context ID.
[3:58:29 PM] [DAP] [Info] ==> {"seq":4861,"type":"event","event":"thread","body":{"reason":"exited","threadId":6}}
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Issuing $dartHotRestartDwds request
[3:58:29 PM] [VmService] [Info] [Flutter (Chrome)] <== {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateExit","timestamp":1646755109861,"isolate":{"type":"@Isolate","id":"6201","number":"6201","name":"main()","isSystemIsolate":false}}}}


# User removes breakpojnt in the IDE (at this point there are no isolates because the previous isolate has exited, so this just updates the local list of breakpoints
# to be empty)

[3:58:29 PM] [DAP] [Info] <== {"command":"setBreakpoints","arguments":{"source":{"name":"package:gallery/…/shrine/login.dart","path":"/Users/danny/Dev/Google/flutter_gallery/lib/studies/shrine/login.dart","sourceReference":0,"adapterData":{"type":"@Script","id":"1940","uri":"package:gallery/studies/shrine/login.dart"}},"lines":[],"breakpoints":[],"sourceModified":false},"type":"request","seq":46}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4862,"type":"response","request_seq":46,"command":"setBreakpoints","success":true,"body":{"breakpoints":[]}}


[3:58:29 PM] [DAP] [Info] ==> {"seq":4864,"type":"event","event":"output","body":{"category":"stdout","output":"[  +72 ms] ChromeProxyService: Initializing expression compiler for main_module.bootstrap.js with sound null safety: false\n"}}
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [  +72 ms] ChromeProxyService: Initializing expression compiler for main_module.bootstrap.js with sound null safety: false
[3:58:29 PM] [DAP] [Info] ==> {"seq":4866,"type":"event","event":"output","body":{"category":"stdout","output":"[  +17 ms] DwdsVmClient: $dartHotRestartDwds request complete.\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4868,"type":"event","event":"output","body":{"category":"stdout","output":"[        ] DwdsVmClient: Waiting for Isolate Start event.\n"}}
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [  +17 ms] DwdsVmClient: $dartHotRestartDwds request complete.
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [        ] DwdsVmClient: Waiting for Isolate Start event.
[3:58:29 PM] [DAP] [Info] ==> {"seq":4870,"type":"event","event":"output","body":{"category":"stdout","output":"[  +21 ms] DwdsVmClient: Successful hot restart\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4877,"type":"event","event":"dart.progressEnd","body":{"progressID":"flutter-f88ed922-3067-4475-ba1c-55cdb5459e9d-hot.restart","message":"Hot Restart complete!"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4879,"type":"event","event":"output","body":{"category":"stdout","output":"[   +1 ms] Restarted application in 260ms.\n"}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4880,"type":"event","event":"thread","body":{"reason":"started","threadId":7}}
[3:58:29 PM] [DAP] [Info] ==> {"seq":4883,"type":"event","event":"dart.serviceExtensionAdded","body":{"extensionRPC":"ext.flutter.disassemble","isolateId":"7231"}}


# Restart completed

[3:58:29 PM] [DAP] [Info] ==> {"seq":4885,"type":"response","request_seq":45,"command":"hotReload","success":true}
[3:58:29 PM] [FlutterRun] [Info] [Flutter (Chrome)] <== [  +21 ms] DwdsVmClient: Successful hot restart


# The old removed breakpoint is added to the new isolate

[3:58:29 PM] [VmService] [Info] [Flutter (Chrome)] <== {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointAdded","timestamp":1646755109972,"isolate":{"type":"@Isolate","id":"7231","number":"7231","name":"main()","isSystemIsolate":false},"breakpoint":{"type":"Breakpoint","id":"bp/8140#170","breakpointNumber":8260,"enabled":true,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","id":"8140","uri":"package:gallery/studies/shrine/login.dart"},"tokenPos":90278}}}}}

[annagrin]

Thanks @DanTup for reporting this! I removed my previous message because I realized I need to experiment with it a bit first:) will ask more questions when I know what potential solutions are.

[annagrin]

@DanTup a question -

User removes breakpoint in the IDE (at this point there are no isolates because the previous isolate has exited, so this
just updates the local list of breakpoints to be empty)

Is there a call to vm_service.removeBreakpoint after the user removes the breakpoint in the IDE? It should remove the breakpoint from _disabledBreakpoint list, so I am surprised that it is still there and therefore re-added after hot restart.

If there is not such call, dwds would not know that the breakpoint needs to be removed. Maybe we can then add it in dart-code, even if there are no current isolates?

[DanTup]

Is there a call to vm_service.removeBreakpoint after the user removes the breakpoint in the IDE? It should remove the breakpoint from _disabledBreakpoint list, so I am surprised that it is still there and therefore re-added after hot restart.

Oh, that's a good question. There is not in the log above, because the previous isolate has exited, and the new isolate has not started. So there is no isolate to send a breakpoint for.

Is this a difference with web versus the VM? My assumption is that for normal VM:

• Hot Reload does not change the isolate, so the breakpoints just remain as they were set. If the user removes one, VS Code/DAP calls removeBreakpoint for that isolate, and everything works
• Hot Restart does change isolate, but flutter_tools does not persist them, and expects the IDE to re-sent breakpoints when the new isolate starts (PauseStart). This means if the user has removed the breakpoint, the IDE will never set that breakpoint on the new isolate

It seems like here, dwds is trying to preserve breakpoints itself across the old/new isolate, which is what causes the issue - because VS Code/DAP does not know about this - it believes breakpoints only exist that it has sent (which are per-isolate).

We perhaps could change VS Code/DAP to understand breakpoints that are added that it did not request, although I don't know how simple that'll be without doing digging (or whether it might change any other behaviour), although I wonder whether dwds behaviour should try to match the VM?

[annagrin]

Is this a difference with web versus the VM? My assumption is that for normal VM:

• Hot Reload does not change the isolate, so the breakpoints just remain as they were set. If the user removes one, VS Code/DAP calls removeBreakpoint for that isolate, and everything works

Web does not support hot reload yet (ie both buttons just hot restart)

• Hot Restart does change isolate, but flutter_tools does not persist them, and expects the IDE to re-sent breakpoints when the new isolate starts (PauseStart). This means if the user has removed the breakpoint, the IDE will never set that breakpoint on the new isolate

Some questions to help clarify the intended behavior:

@bkonyi do you know if this is the expected behavior of the VM?

I was not aware that IDEs track breakpoints between hot restarts.
@helin24 @elliette do IntelliJ, DevTools do the same?

@grouma do you remember by any chance why we needed to keep track of breakpoints in dwds between hot restarts? Do we have vm service uses without IDEs that can set/remove dart breakpoints and hot restart?

Dwds (which is running inside flutter tools) keeps track of breakpoints currently. We remove all breakpoints before hot restart so hot restart can continue, and put them back right after hot restart has finished and a new isolate is created. So if a breakpoint is removed in-between isolates, dwds would need to see a removeBreakpoint call to remove it (previous isolate ID is ok to use).

Depending on the answers to the questions above, we can remove the tracking altogether or keep it as is.
In the meanwhile, I can prevent the issue by not allowing to change breakpoints when there is no isolate present - @DanTup does it look like a good workaround?

[DanTup]

In the meanwhile, I can prevent the issue by not allowing to change breakpoints when there is no isolate present - @DanTup does it look like a good workaround?

I don't think this will work for a few different reasons:

1. as I understand it, the IDE is not actually sending a remove breakpoint request, because the isolate that had the breakpoint has exited (and breakpoints are isolate-specific, so if we have 0 isolates, we believe we have 0 breakpoints). So there's no request actually going to the proxy to reject.
2. if it was possible for dwds to reject it, the DAP protocol doesn't allow for us to do something other than what the IDE requested (eg. if it says "replace the list of breakpoints for this file with this new set", we can't return it a different or empty set). It's possible we could reject the whole request and it might keep the state as it was, but since we call the VM Service for each isolate individually, this could still become inconsistent (eg. if some of the requests work and some do not)

[DanTup]

Actually, DAP does allow us to send breakpoint events to the editor.. In theory, we could detect the new breakpoint being re-added (assuming there's an event for it) and send it back to the editor, so it would show up again (so the UI is in-sync with the VM Service). But it would require some work to support that and it would still result in a sub-optimal experience ("I removed a breakpoint and it came back a few seconds later") so if we can fix it a better way, I think that would be better.

I wonder if the reason for the breakpoints being persisted is because web pretends to hot reload (where an IDE expects them to persist and doesn't re-send them) but actually restarts?

I've never really been a fan of that behaviour though, because it means the user has two buttons in the IDE labelled Hot Reload and Hot Restart that both do the same thing. This is pretty confusing IMO (users may wonder why Hot Reload isn't preserving state as they're taught it should). I think originally the Hot Reload service was not loaded, and in VS Code this would disable Hot Reload and we changed the on-save behaviour to fall back to Hot Restart - that seemed much clearer to me (for ex. the progress notification when saving would show "Hot Restarting..." instead of "Hot Reloading...").

(I also think changing the isolate ID when the IDE things it's just hot reloading could lead to other issues, because the IDE may be tracking which isolates it has sent breakpoints for - although I'd have to test/check the code to understand the implications of that)

[bkonyi]

@bkonyi do you know if this is the expected behavior of the VM?

Hot restart creates a completely new isolate, so breakpoints aren't persisted. Hot reload replaces modified script objects in the VM. Since breakpoints are tied to these objects and the VM doesn't know how to re-map these breakpoints, breakpoints aren't persisted in modified files after a hot reload. It's up to the client to reset these breakpoints (part of the reason we added the client synchronization capabilities to DDS).

[DanTup]

Since breakpoints are tied to these objects and the VM doesn't know how to re-map these breakpoints, breakpoints aren't persisted in modified files after a hot reload

Apparently my understanding was not correct. I thought they persisted over reloads. Now I'm not certain VS Code does the right thing (I think it only re-sends them for Hot Restart) - I'll do some testing!

[annagrin]

@bkonyi do I understand correctly that to be consistent with VM, dwds should clear all breakpoints on hot restart and not bring them back after?

I think there would be currently a problem in internal scenarios where we do not clear and reestablish breakpoints but just let chrome keep track of them. Those scenarios would need to be fixed as well (ideally to invoke the same functionality as flutter tools for web does).

@elliette FYI

[bkonyi]

That's correct. The service doesn't have enough context to know where to put breakpoints since scripts could have changed significantly after reload, completely changing the behavior of breakpoints inserted at the same lines.

[DanTup]

Turns out I was mis-remembering. During a hot reload, we still get paused isolates (PausePostRequest) which causes us to re-send breakpoints in VS Code. So in both cases (hot reload and hot restart), VS Code will re-send breakpoints.

Although, I did find one weird case where if you do not reload, but you modify a file and save it, the IDE will send the new breakpoint locations even though the VM is still running old code. So editing files and not reloading can do unexpected things with breakpoints - but I'm not sure there's a reasonable fix there (we have no way of knowing from the debug adapter whether breakpoints being sent by the IDE correspond to the same version of the file in the VM, or a newly modified one that has not been reloaded yet).

(TL;DR, VS Code does re-send breakpoints on a hot reload too and this tangent was not all that relevant 🙃).

[annagrin]

@DanTup I will try to remove the breakpoint tracking from dwds and see if it breaks any of our scenarios. Will report back!

[annagrin]

@DanTup apologies for the delay! I've tried removing tracking from dwds but that does not work in scenarios without and editor like VSCode or IntelliJ to control the debugging (i.e. in flutter run -d chrome and dart DevTools for debugging.)

@elliette @kenzieschmoll do you know how this works when running flutter tools for non-web platforms and using DevTools to debug without another editor? Does DevTools track breakpoints for other platforms but not for web?

[DanTup]

This came up again in Flutter discord today (or at least, we believe it's the same issue).

I had a quick look in DevTools code and found some code that's updating breakpoints after a reload:

https://github.com/flutter/devtools/blob/3df73804d235eae260d9584a905df0ba08f75db7/packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart#L130

However I can't see anything for a Hot Restart. I tested this running the counter app on iOS Simulator, and the breakpoints disappear on a hot restart.

So as far as I can tell, that is normal behaviour for DevTools, and not something DWDS needs to handle for web, since DevTools will need to do it to support all platforms?

[elliette]

Closing as duplicate of #2257