Ensure debug adapter request/response sequence numbers tally#14336
Ensure debug adapter request/response sequence numbers tally#14336
Conversation
joyceerhl
requested review from
DavidKutu,
DonJayamanne,
IanMatthewHuff and
rchiodo
|
|
||
| // When a python debugging session is active keep track of the current debug location | ||
| export class DebugLocationTracker implements DebugAdapterTracker { | ||
| export class DebugLocationTracker extends DebugStackTraceTracker implements DebugAdapterTracker { |
There was a problem hiding this comment.
Do we still need implements DebugAdapterTracker?
There was a problem hiding this comment.
Yes, because DebugLocationTracker has other custom message handling besides what is handled in the base class, so it still needs to be able to intercept DAP messages via onDidSendMessage and onWillReceiveMessage.
| message.request_seq === this.stackFrameRequestSequenceNumber | ||
| ) { | ||
| // This should be the top frame. We need to use this to compute the value of a variable | ||
| this.topMostFrameId = message.body.stackFrames[0].id; |
There was a problem hiding this comment.
This feels inconsistent because this state is only needed in debuggerVariables.ts, but here I've made it shared, and the equivalent state for debugLocationTracker.ts still lives in that class and not here.
Wanted people's thoughts on whether I should either
- move debugLocationTracker.ts's handling for a stackTrace response here, or
- move this function back into debuggerVariables
There was a problem hiding this comment.
This class's job is to track the list of stack frames. That shouldn't be done elsewhere. I'd say you could probably put almost all (if not all) of the code from the DebugLocationTracker here. Isn't the DebuggerVariable class really just a subclass of something that's tracking stack frames?
There was a problem hiding this comment.
Meaning you'd have this class hierarchy instead:
DebugVariables extends DebugLocationTracker
DebugLocationTracker
There was a problem hiding this comment.
Given the name / function of this class as I read it #1 makes more sense.
| } | ||
|
|
||
| public onWillReceiveMessage(message: DebugProtocol.Request) { | ||
| if (message.type === 'request' && message.command === 'stackTrace' && message.arguments.startFrame === 0) { |
There was a problem hiding this comment.
From the specification this is not quite right I believe. Looks like startFrame === 0 or startFrame not being defined means to fetch everything. I think this might not handle the undefined case.
There was a problem hiding this comment.
Good catch, I missed that when looking at the spec. Thank you!
| private sessionEndedEmitter: EventEmitter<DebugLocationTracker> = new EventEmitter<DebugLocationTracker>(); | ||
|
|
||
| constructor(private _sessionId: string) { | ||
| constructor(private _sessionId: string | undefined) { |
There was a problem hiding this comment.
This is a bit of a cheat to get the class inheritance to work @rchiodo, because in debuggerVariables we may not have an active session at the time that the constructor is called. debuggerVariables doesn't need this to work anyway, this sessionId is used in the debugLocationTrackerFactory, but just to flag it here in case folks think this looks funny.
…o dap-sequence-numbers
…o dap-sequence-numbers
Codecov Report
@@ Coverage Diff @@
## main #14336 +/- ##
==========================================
- Coverage 59.40% 59.40% -0.01%
==========================================
Files 716 716
Lines 39999 40000 +1
Branches 5793 5793
==========================================
- Hits 23763 23762 -1
- Misses 14975 14976 +1
- Partials 1261 1262 +1
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed!
|
* Ensure debug adapter request/response sequence numbers tally (#14336) * Port issue assigning fixes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14336 +/- ##
======================================
- Coverage 59% 59% -1%
======================================
Files 716 716
Lines 39999 40000 +1
Branches 5793 5793
======================================
- Hits 23763 23762 -1
- Misses 14975 14976 +1
- Partials 1261 1262 +1
🚀 New features to boost your workflow:
|
7 participants
While working on #14269, I found a bug in how we update
topMostFrameIdindebuggerVariables.ts. VSCode may send multiple stackTrace requests, but we should only updatetopMostFrameIdwhen we get a response to a request where thestartFramewas 0 (since that will retrieve all frames on the stack).In #14269, this was causing variables to appear undefined since they were being evaluated with respect to the wrong frame. @rchiodo pointed out that we needed to fix this in
debugLocationTracker.tsas well, so I created a base class to track the sequence numbers since that code is needed across bothdebuggerVariables.tsanddebugLocationTracker.ts.I didn't want to move more state being tracked as part of the stackTrace response in
debugLocationTracker.tsinto the base class, since there seems to be some custom logic there WRT whether we're waiting for a stack trace, but if that is safe and not messy to move into a shared class then I can make that change.package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).