Configure DevTools to pause isolates on hot restart, and then only resume once breakpoints have been set #7234
Conversation
packages/devtools_app_shared/lib/src/service/service_manager.dart
Outdated
Show resolved
Hide resolved
| if (service != null) { | ||
| try { | ||
| final vmService = service!; | ||
| await vmService.setFlag('pause_isolates_on_start', 'true'); |
There was a problem hiding this comment.
We need to be a bit careful if we're going to set this flag.
If the user starts their process without --pause_isolates_on_start set, they connect to DevTools, and then their application spawns a new isolate, they're going to be surprised if their isolate pauses on start.
Similarly, if the user starts their application with --pause_isolates_on_start, they connect to DevTools, and their program spawns another isolate, they'll be surprised if DevTools automatically resumes the newly spawned isolate.
We'll want to test both these cases to make sure we can handle them correctly. I'm fairly certain that the second case will be broken with this PR, but I'm not 100% sure about the first case.
There was a problem hiding this comment.
Hmm these are good points.
I've pushed up changes to resolve the first case:
"If the user starts their process without --pause_isolates_on_start set, they connect to DevTools, and then their application spawns a new isolate, they're going to be surprised if their isolate pauses on start."
Regarding the second case:
"Similarly, if the user starts their application with --pause_isolates_on_start, they connect to DevTools, and their program spawns another isolate, they'll be surprised if DevTools automatically resumes the newly spawned isolate."
I think we can handle that case by DevTools getting the flag list from the VM to check if pause-isolates-on-start flag has already been set. However, I think there is another issue. If a user is starting their application with --pause_isolates_on_start, they will also be surprised if their main isolate is resumed by DevTools (after the breakpoints have been set). There is no way for DevTools to tell if the flag was set by another debugging client (e.g. DAP), in which case as long as we are using requirePermissionsToResume it is safe to resume the isolate, or if the flag was set by a user, in which case resuming the isolate would cause unexpected behavior. Adding @DanTup as well since whatever is done here will also need to be done at https://dart-review.googlesource.com/c/sdk/+/350649 (will get back to that CL eventually I promise!!)
One idea: if a user starts their app with pause-isolates-on-start, could we also automatically set requirePermissionsToResume?
There was a problem hiding this comment.
I think we can handle that case by DevTools getting the flag list from the VM to check if pause-isolates-on-start flag has already been set.
Hmm, what happens if the user started their app via VS Code, and it set pause-on-start. DevTools wouldn't know whether the user has set it (and expects them to stay paused), or if VS Code did it (and is auto-resuming them, so the user expects them not to stay paused)?
In DAP we have some similar handling for pause_on_exit... We automatically add pause-on-exit if it's not there (so we can drain output events etc. before the VM terminates), but if the user had already passed that flag in explicitly, we keep a flag to ensure we don't auto-resume when we're done. This only works when we launch the app though, I suspect attach might have similar edge cases.
One idea: if a user starts their app with pause-isolates-on-start, could we also automatically set requirePermissionsToResume?
requirePermissionsToResume is client-specific. If it gets set automatically for a client that doesn't want to handle resuming, I think things would end up indefinitely paused?
There was a problem hiding this comment.
"Hmm, what happens if the user started their app via VS Code, and it set pause-on-start. DevTools wouldn't know whether the user has set it (and expects them to stay paused), or if VS Code did it (and is auto-resuming them, so the user expects them not to stay paused)?"
Yes, that's the problem. I'm not sure what a good solution is.
If both DevTools and DAP set pause-isolates-on-start, they can use requirePermissionsToResume(onStart) to both safely resume, knowing that DDS will wait until they have both resumed to send the resume event.
However, if the user has also set pause-isolates-on-start, there is (as far as I know) no way for DevTools and DAP to know that it was set by the user and not by another debugging client. In which case both of them using requirePermissionsToResume(onStart) is not sufficient, because they would both send the resume event and DDS would not wait for the user to also resume.
That's why I'm proposing that when a user sets pause-isolates-on-start, requirePermissionsToResume is automatically called for them. Basically, DDS would treat the user like another debugging client, and wait for DevTools, DAP, and the user to all resume before the VM service resumes.
There was a problem hiding this comment.
and the user to all resume before the VM service resumes
I think the issue here is that the user doesn't have a connection to the VM, so they can't "resume". The users request to resume goes via DAP (or DevTools) and looks to DDS as a request from them. I think to do this, we'd need to distinguish between a user-initiated resume and a tool asking to resume because it's done with its configuration?
There was a problem hiding this comment.
Ok - I think this is finally ready for review.
The second case is now handled:
""Similarly, if the user starts their application with --pause_isolates_on_start, they connect to DevTools, and their program spawns another isolate, they'll be surprised if DevTools automatically resumes the newly spawned isolate."
However the first case is not (as discussed offline, this requires DDS to set requireUserPermissionsToResume to true on start up if the flag pause-isolates-on-start is set). However, that change has been made to DDS this will work as well.
There was a problem hiding this comment.
Now that dart-lang/sdk@9382e58 has landed and DDS 4.1.0 has been published the first case is now handled as well.
packages/devtools_app_shared/lib/src/service/isolate_manager.dart
Outdated
Show resolved
Hide resolved
| final service = _service!; | ||
| try { | ||
| await service.readyToResume(isolateId); | ||
| } on UnimplementedError { |
There was a problem hiding this comment.
Does this actually throw UnimplementedError on this end? I would assume this would actually result in an RPCError with code kMethodNotFound.
There was a problem hiding this comment.
Just verified that is does throw UnimplementedError (for both web and non-web apps)
|
@maxfrees it's hard to say without more information. Does it occur in a newly created project? Are you able to share exact steps that would help reproduce the issue? It's probably better to file a new issue with the details. |
Work towards #7231