Skip to content

DebugAdapterException thread was not found during flutter debug-adapter #4515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
christopherfujino opened this issue Apr 27, 2023 · 10 comments
Closed
Labels
important Something important! in debugger Relates to the debug adapter or process of launching a debug session is bug
Milestone

Comments

@christopherfujino
Copy link

Describe the bug

Flutter Beta 3.10.0-1.4.pre (with dds v2.7.9) was just released yesterday April 26. There is one crash with a DebugAdapterException:

DebugAdapterException: DebugAdapterException: Thread 107 was not found
at IsolateManager.resumeThread(isolate_manager.dart:263)
at IsolateManager._handlePause(isolate_manager.dart:479)
at <asynchronous gap>(async)
at IsolateManager.handleEvent(isolate_manager.dart:178)
at <asynchronous gap>(async)
at DartDebugAdapter.handleDebugEvent(dart.dart:2076)
at <asynchronous gap>(async)
at DartDebugAdapter._withErrorHandling(dart.dart:2417)
at <asynchronous gap>(async)

To Reproduce
flutter debug_adapter

Please complete the following information:

  • Operating System and version: Windows [Version 10.0.22621.1631]
  • VS Code version: unknown
  • Dart extension version: unknown
  • Dart/Flutter SDK version: Flutter 3.10.0-1.4.pre
  • Target device (if the issue relates to Flutter debugging): Unknown
@DanTup
Copy link
Member

DanTup commented Apr 27, 2023

Hmm, this one is a little odd. This error is thrown when we can't find a thread using an ID we assigned. As far as I can tell, the only way that happens is if we have already processed the IsolateExit event.

So I suspect here we got both an IsolatePause and an IsolateExit event, but maybe due to awaits, we finished processing the IsolateExit before we finished processing the pause. However, we shouldn't really get an IsolateExit if an isolate was paused, because we haven't unpaused it yet.

So like with one of the previous fixes, my feeling is that either:

  • This is happening during shutdown, so the Isolate exited because the app was closing (and not that it was unpaused and finished)
  • There are two debuggers attached, and the other one unpaused the isolate (and it exited) while we were trying to handle the pause

I'll add some handling for this anyway. We should probably keep knowledge of exited events around (similar to the VM keeping Sentinels) and that way we can be sure when we're just trying to process an exited isolate (in which case we can silently do nothing) as opposed to the client sent us a bogus thread ID and we want to fail the request.

@DanTup DanTup added the in debugger Relates to the debug adapter or process of launching a debug session label Apr 27, 2023
@DanTup DanTup added this to the v3.64.0 milestone Apr 27, 2023
@DanTup DanTup added the important Something important! label Apr 27, 2023
@christopherfujino
Copy link
Author

There are two debuggers attached, and the other one unpaused the isolate (and it exited) while we were trying to handle the pause

Is having multiple debuggers attached supported? Could we have a second debugger attaching somehow notify the first?

@DanTup
Copy link
Member

DanTup commented Apr 27, 2023

It doesn't work properly today, but there are some new APIs each debugger can start using to help. For example when a new isolate starts (paused) we normally send our breakpoints and then unpause it. For multiple debuggers, they need to instead send their breakpoints and then tell DDS they are ready for it to be unpaused (and DDS will unpause it when all clients are ready).

It requires all of the connected debuggers use those new APIs though. Currently (as far as I know), none of legacy DAP, new DAP, IntelliJ or DevTools implement these APIs.

If you see a lot of these, then I suspect it's more likely a race during shutdown (or a restart) because I don't expect many people are attaching two debuggers. Hard to say for sure without a repo, but there's no reason not to change this code to treat exited isolates different to invalid IDs (currently it assumes any ID it can't find is invalid and it should throw), and then all calling code just needs to decide what to do when an isolate it wanted to interact with has since exited (most likely by just doing nothing).

I should be able to get a fix for this tomorrow. I don't know whether it makes most sense to try and CP immediately, or try to get a better idea of how often it's happening though?

@christopherfujino
Copy link
Author

christopherfujino commented Apr 27, 2023

I should be able to get a fix for this tomorrow. I don't know whether it makes most sense to try and CP immediately, or try to get a better idea of how often it's happening though?

I think our options are either get a CP out ASAP or risk not enabling DAP on stable.

@DanTup
Copy link
Member

DanTup commented Apr 28, 2023

I have a fix open at https://dart-review.googlesource.com/c/sdk/+/299640.

If we want to cherry-pick this, we need a DDS release that matches the 2.7.9 release + this fix, for ex:

git checkout facfcf0f92ed28173f4fad9a3d21a90409582367
git cherry-pick [hash of newly landed change]
pub publish --dry-run (etc.)

If we just publish from master we would get some refactoring from dart-lang/sdk@5ef021b that we probably don't want to risk including.

@bkonyi FYI

@christopherfujino
Copy link
Author

If we just publish from master we would get some refactoring from dart-lang/sdk@5ef021b that we probably don't want to disk including.

Agreed, I don't believe a hotfix rolling DDS to latest will be approved.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue May 1, 2023
There are two cases where we might be trying to resume an exited isolate:

1. Another debugger resumed the isolate and it exited before we got here
2. We were starting to resume an isolate but then the app was shut down so the isolate exited

Because of async code, we can never guarantee the isolate hasn't exited before we send this event, so the exception we throw should only ever be because the ID was invalid.

Fixes Dart-Code/Dart-Code#4515.

Change-Id: If3500d5d1adf3ba9179e6a571130256783b23c2d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/299640
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
@bkonyi
Copy link

bkonyi commented May 1, 2023

I've published 2.7.10 based on facfcf0f92ed28173f4fad9a3d21a90409582367 with 14ab84ebb30f180f46acec3e1d9a59d680622530 cherry picked.

@DanTup
Copy link
Member

DanTup commented May 1, 2023

@bkonyi thank you!

The bot has opened flutter/flutter#125801 to roll this into master, which will be the change to cherry-pick.

@christopherfujino
Copy link
Author

Thanks @bkonyi & @DanTup, I'll work on ensuring this is merged to master and file the request to CP 2.7.10 to beta.

@DanTup
Copy link
Member

DanTup commented May 1, 2023

@christopherfujino thanks! Closing this since it's fixed, but please let me know if you see any other issues cropping up in the beta.

@DanTup DanTup closed this as completed May 1, 2023
CaseyHillers pushed a commit to flutter/flutter that referenced this issue May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
important Something important! in debugger Relates to the debug adapter or process of launching a debug session is bug
Projects
None yet
Development

No branches or pull requests

3 participants