Skip to content
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

[debug]: Cannot stop debug session of vscode-mock-debug #11879

Closed
kittaakos opened this issue Nov 18, 2022 · 5 comments · Fixed by #11984
Closed

[debug]: Cannot stop debug session of vscode-mock-debug #11879

kittaakos opened this issue Nov 18, 2022 · 5 comments · Fixed by #11984
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility

Comments

@kittaakos
Copy link
Contributor

kittaakos commented Nov 18, 2022

Bug Description:

It's impossible to terminate the debug session in Theia. It works very well from Code.

Steps to Reproduce:

  1. Start the Microsoft/vscode-mock-debug debugger in Theia.
  2. Stop the debug session.
  3. It cannot terminate and will timeout after 5 sec
2022-11-18T11:59:05.246Z root ERROR Error on disconnect
{
    "type": "response",
    "seq": 0,
    "request_seq": 12,
    "success": false,
    "command": "disconnect",
    "message": "Request #12: disconnect timed out"
}

If you do not want to build the vscode-mock-debug extension on your own, you can use the VS Code extension mock debugger from #11871

Additional Information

  • Operating System: macOS, 12.5.1
  • Theia Version: fe8e4c1
@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility debug issues that related to debug functionality labels Nov 21, 2022
@kittaakos
Copy link
Contributor Author

I will handle this.

@kittaakos
Copy link
Contributor Author

After microsoft/vscode#166137, I can confirm Theia's behavior is the same as Code.

I was positive the debug session's fast termination was due to no await in Code's implementation, then microsoft/vscode#166137 popped up. I debugged into the non-minified version of the mock-debug implementation, and I can confirm no disconnect response was sent back to the client. So a timeout is expected. See here. I downloaded latest Code Insider, and now I can see the 2-sec delay after terminating the debug session. The differences are that Theia uses 5000ms while Code 2000ms. Code ignores errors, Theia does not.

Theia should adjust the UX, though. Shall Theia ignore the error and timeout after 2000ms (or make the timeout easily configurable from downstream code)?

We can close the issue. It's up to core committers. Thanks!

@msujew
Copy link
Member

msujew commented Nov 21, 2022

Code ignores errors, Theia does not.

Note that Theia also "ignores" the error. I wouldn't say that logging the error to console really counts as error handling. We don't show anything to the user or anything similar.

@kittaakos
Copy link
Contributor Author

kittaakos commented Nov 22, 2022

Note that Theia also "ignores" the error.

Yes, thanks for the correction. What I meant is logging the error is misleading. No malfunction is happening when terminating the debug session with 'disconnect' and there is a timeout.

@kittaakos
Copy link
Contributor Author

Let's leave this open. There are other places we could improve Theia. For example, when a request is timed out, the error object used for rejection does not have an error message. However, DAP has a spacial ErrorResponse, Theia could benefit from it:

	/** On error (whenever `success` is false), the body can provide more details. */
	interface ErrorResponse extends Response {
		body: {
			/** A structured error message. */
			error?: Message;
		};
	}

Current code:

const error: DebugProtocol.Response = {
type: 'response',
seq: 0,
request_seq: request.seq,
success: false,
command,
message: `Request #${request.seq}: ${request.command} timed out`
};
pendingRequest.reject(error);

Also, it should be easier for downstream to specify a timeout for terminate and disconnect. I would also like to increase the visibility of the isStopped to improve the APIs for downstream:

private isStopping: boolean = false;

kittaakos pushed a commit to kittaakos/theia that referenced this issue Dec 13, 2022
 - feat: aded support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (eclipse-theia#11871),
 - feat: can customize debug session timeout, and error handling (eclipse-theia#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916)

Closes eclipse-theia#11871
Closes eclipse-theia#11879
Closes eclipse-theia#11885
Closes eclipse-theia#11886
Closes eclipse-theia#11916

Signed-off-by: Akos Kitta <[email protected]>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Dec 13, 2022
 - feat: aded support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (eclipse-theia#11871),
 - feat: can customize debug session timeout, and error handling (eclipse-theia#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916)

Closes eclipse-theia#11871
Closes eclipse-theia#11879
Closes eclipse-theia#11885
Closes eclipse-theia#11886
Closes eclipse-theia#11916

Signed-off-by: Akos Kitta <[email protected]>

s

Signed-off-by: Akos Kitta <[email protected]>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Dec 13, 2022
 - feat: aded support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (eclipse-theia#11871),
 - feat: can customize debug session timeout, and error handling (eclipse-theia#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916),
 - fix: validate editor selection based on the text model (eclipse-theia#11880)

Closes eclipse-theia#11871
Closes eclipse-theia#11879
Closes eclipse-theia#11885
Closes eclipse-theia#11886
Closes eclipse-theia#11916
Closes eclipse-theia#11880

Signed-off-by: Akos Kitta <[email protected]>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Dec 13, 2022
 - feat: added support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (eclipse-theia#11871),
 - feat: can customize debug session timeout, and error handling (eclipse-theia#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916),
 - fix: validate editor selection based on the text model (eclipse-theia#11880)

Closes eclipse-theia#11871
Closes eclipse-theia#11879
Closes eclipse-theia#11885
Closes eclipse-theia#11886
Closes eclipse-theia#11916
Closes eclipse-theia#11880

Signed-off-by: Akos Kitta <[email protected]>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Jan 17, 2023
 - feat: added support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (eclipse-theia#11871),
 - feat: can customize debug session timeout, and error handling (eclipse-theia#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (eclipse-theia#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (eclipse-theia#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (eclipse-theia#11916),
 - fix: validate editor selection based on the text model (eclipse-theia#11880)

Closes eclipse-theia#11871
Closes eclipse-theia#11879
Closes eclipse-theia#11885
Closes eclipse-theia#11886
Closes eclipse-theia#11916
Closes eclipse-theia#11880

Signed-off-by: Akos Kitta <[email protected]>
paul-marechal added a commit that referenced this issue Jan 24, 2023
* fix: various debug fixes and VS Code compatibility enhancements

 - feat: added support for `debug/toolbar` and `debug/variables/context`,
 - feat: added support for `debugState` when context (#11871),
 - feat: can customize debug session timeout, and error handling (#11879),
 - fix: the `debugType` context that is not updated,
 - fix: `configure` must happen after receiving capabilities (#11886),
 - fix: added missing conext menu in the _Variables_ view,
 - fix: handle `setFunctionBreakboints` response with no `body` (#11885),
 - fix: `DebugExt` fires `didStart` event on `didCreate` (#11916),
 - fix: validate editor selection based on the text model (#11880)

Signed-off-by: Akos Kitta <[email protected]>

* fix: use when context for command node filtering

Signed-off-by: Akos Kitta <[email protected]>

* chore: removed test debug VSIX

Signed-off-by: Akos Kitta <[email protected]>

* fix: revert `timeout` check

Signed-off-by: Akos Kitta <[email protected]>

* fix: clarification on the possible `'debugState'`

Signed-off-by: Akos Kitta <[email protected]>

* fix: use hard-coded debugger `clientID` and `clientName`

Signed-off-by: Akos Kitta <[email protected]>

* fix: use review-requested method name

Signed-off-by: Akos Kitta <[email protected]>

* fix: changed method name + added doc

Signed-off-by: Akos Kitta <[email protected]>

* fix: `stopTimeout` is a default `ctor` argument

Signed-off-by: Akos Kitta <[email protected]>

* fix: incorrect method name

Signed-off-by: Akos Kitta <[email protected]>

* fix: both `didCreate` and `didStart` must be API

Signed-off-by: Akos Kitta <[email protected]>

* fix: call both on create and start

Signed-off-by: Akos Kitta <[email protected]>

* fix: workaround for microsoft/vscode-mock-debug#85

Signed-off-by: Akos Kitta <[email protected]>

* simplify writing

The collection of contributed commands was written in a convoluted way,
this commit makes it more straightforward with just 2 loops.

* use `Math.max` to clamp values into positives

The ternary implementation is stricly equivalent to the `Math.max`
function, so use that instead.

* fix default option value handling

Assigning a default option object to set default values is problematic
as said default values will be discarded if any option object is passed.

Instead default values must be handled in conjuction of whatever options
are passed to functions.

Signed-off-by: Akos Kitta <[email protected]>
Co-authored-by: Paul Maréchal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants