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

Invoking open-trace-with-path command via vscode.commands.executeCommand() shows "maximum call stack size exceeded" error #1096

Open
GregSavin opened this issue Jul 24, 2024 · 17 comments

Comments

@GregSavin
Copy link

Steps to replicate issue:

  1. In a source file within a VSCode extension that uses theia-trace-extension (version 0.2.0), establish a test command handler using this as a template:

    context.subscriptions.push(vscode.commands.registerCommand('test.command', () => {
    let tracePath = '/path/to/folder/with/valid/CTF/trace'; // substitute a real, existent, and valid path here
    vscode.commands.executeCommand("open-trace-with-path", tracePath);
    }));

  2. Add the corresponding test command to the "contributes" -> "commands" section of your package.json, e.g.:

    {
    "command": "test.command",
    "title": "Test Command"
    }

  3. Build your Theia-based platform, including the VSCode extension implied above.

  4. Launch your Theia-based platform in such a way that you retain visibiity into a terminal/console that would receive any electron console logs and error messages.

  5. Within Theia-based platform, invoke the test command by choosing it from the command list that pops up in response to a keystroke.

  6. Observe the trace successfully opened in the UI.

  7. Also observe an error message, sent to console, of the general form:

2024-07-24T15:56:52.238Z root ERROR [hosted-plugin: 101070] Promise rejection not handled in one second: Error: Error during encoding: 'Maximum call stack size exceeded' , reason: Error: Error during encoding: 'Maximum call stack size exceeded'
2024-07-24T15:56:52.239Z root ERROR [hosted-plugin: 101070] With stack trace: Error: Error during encoding: 'Maximum call stack size exceeded'
at MsgPackMessageEncoder.encode (file:///my-theia-platform/examples/electron/lib/frontend/bundle.js:154835:23)
at MsgPackMessageEncoder.replyOK (file:///my-theia-platform//examples/electron/lib/frontend/bundle.js:154824:14)
at RpcProtocol.handleRequest (file:///my-theia-platform//examples/electron/lib/frontend/bundle.js:155055:26)

@marcdumais-work
Copy link
Contributor

Interesting use case @GregSavin - you are using the Theia trace viewer, as part of a Theia-based app, but you are (probably among other things) triggering opening a trace from a custom vscode extension?

that uses theia-trace-extension (version 0.2.0)

Do you know if the issue still happens with more recent versions?

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jul 24, 2024

@GregSavin a bit of a shot in the dark, but the error you get is similar to something that was happening relatively recently in Theia apps. Can you report what the following command outputs, when run it in your Theia application folder? (i.e. the same one that has the issue)

yarn why msgpackr

@GregSavin
Copy link
Author

Hi @marcdumais-work,

Interesting use case @GregSavin - you are using the Theia trace viewer, as part of a Theia-based app, but you are (probably among other things) triggering opening a trace from a custom vscode extension?

That is correct.

that uses theia-trace-extension (version 0.2.0)

Do you know if the issue still happens with more recent versions?

We haven't tried more recent versions yet. 0.2.0 might have been the most recent as of the time our project started, but I'll see if we can sync up with more recent versions and see if there is a difference.

@GregSavin
Copy link
Author

@GregSavin a bit of a shot in the dark, but the error you get is similar to something that was happening relatively recently in Theia apps. Can you report what the following command outputs, when run it in your Theia application folder? (i.e. the same one that has the issue)

yarn why msgpackr

Thanks for the info that similar things have been seen generally. Here is the yarn output, in case it gives an additional clue:

(bash)$ yarn why msgpackr
yarn why v1.22.22
[1/4] Why do we have the module "msgpackr"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "msgpackr@1.6.1"
info Reasons this module exists

  • "project#@Theia#core" depends on it
  • Hoisted from "project#@Theia#core#msgpackr"
    info Disk size without dependencies: "360KB"
    info Disk size with unique dependencies: "360KB"
    info Disk size with transitive dependencies: "360KB"
    info Number of shared dependencies: 0
    Done in 0.47s.

@marcdumais-work
Copy link
Contributor

=> Found "msgpackr@1.6.1"

The bug I remembered:
eclipse-theia/theia#12499

According to this (un-merged) Theia PR, that version you currently pull is older than the affected range:
https://github.com/eclipse-theia/theia/pull/12600/files#diff-3ba427921747beae63b7bde68d6ef51ba40352c7ed1cb54cf25c97579aec6d39R16

It's still possible they did not test back older msgpackr versions beyond v1.8.3.

If you're up for it, I would suggest something quick to try: Use an entry in a resolutions block in the Theia app's package.json (or the root package.json if you have the app as part of a monorepo), to pin msgpackr to version to e.g. "^1.10.1" like so:

"resolutions": {
"msgpackr": "^1.10.1",

You can then rebuild and use "yarn why" to confirm that the pulled version is as requested, and see if it helps.

@GregSavin
Copy link
Author

Thanks for the suggestion of pinning msgpackr to ^1.10.1, @marcdumais-work. That sounded promising, and was worth a try, and yarn grabbed 1.11.0, as then reflected in "yarn why msgpackr" output, though it doesn't seem to change the observation at runtime. From what we can tell so far, other than the console error output, things seem to be working fine, functionally (regardless of older vs newer msgpackr). So that's good. I'll follow up on your initial suggestion of trying a newer theia-trace-extension version.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jul 25, 2024

@GregSavin I was able to dig a bit deeper and found something interesting.

This is the method that is invoked when we run the test command - it returns a promise of a TraceViewerWidget:
https://github.com/eclipse-cdt-cloud/theia-trace-extension/blob/v0.2.0/theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-contribution.ts#L147
image

Looking in the debugger just before the exception happens, indeed the rpc encoder chokes trying to encore the "result" of the command, a TraceViewerWidget object. The exception occurs on line 215.
image

I have a feeling that the vscode extension host does not need this widget object in the first place - I am not sure why it's returned - maybe when this command is invoked from the Theia app it's useful? If so, then perhaps it would be needed to have a version of this command that's vscode-invocation-friendly, that returns e.g. a Promise<void>?

@marcdumais-work
Copy link
Contributor

The open() method is shared between different commands, so I think we don't want to modify it. We can however modify the command handler, to explicitly return nothing when it's done. I tried something for a quick test, and it seems to get rid of the encoding exception:

image

If the original implementation is useful, we may instead need to duplicate the command, but you get the idea?

@marcdumais-work
Copy link
Contributor

@bhufmann when you are back from vacation, I would like to get your opinion about this. Do you know that it may be useful in some cases for the OpenTraceWithPathCommand to return the newly opened TraceViewerWidget?

@GregSavin
Copy link
Author

Thanks @marcdumais-work for the detailed investigation and explanation! If I step through the call that you identified, I can see the RPC packing of the response is indeed taking an exception, and this explanation aligns with the observation that the error message is the only top-level-visible unexpected behavior. So practically it's not a significant problem. The UI side effects have all completed as expected by that point. I think you are right that at least in the case of a vscode extension being the caller, a return value doesn't seem to be expected or subsequently acted upon by the vscode extension.

@marcdumais-work
Copy link
Contributor

Note: I did notice a small unexplained glitch when triggering the command to open a specific trace, from a vscode extension: once opened, the trace was named "(1)" in the left-side trace panel view, instead of taking the trace's root folder name like usual:

image

Normally:
image

@bhufmann
Copy link
Collaborator

@bhufmann when you are back from vacation, I would like to get your opinion about this. Do you know that it may be useful in some cases for the OpenTraceWithPathCommand to return the newly opened TraceViewerWidget?

I don't have any use cases for that at the moment. It's not used in our user base. However, the OpenTraceWithPathCommand was contributed by @jonahgraham in commit 1c6babd and I'd like him to weigh in on not returning the TraceViewerWiget when executing this command. @jonahgraham would that be ok with you?

@bhufmann
Copy link
Collaborator

bhufmann commented Aug 16, 2024

@GregSavin would it be an option for you to use the vscode-trace-extension and install it as vscode plugin (from OpenVsx or Marketplace) instead of using the Theia integration theia-trace-extension?

@GregSavin
Copy link
Author

Will give the vscode-trace-extension a try; thanks!

@bhufmann
Copy link
Collaborator

Will give the vscode-trace-extension a try; thanks!

@GregSavin, please note that the viewer vscode extension doens't have the command to start/strop the trace server. You could install the vscode-trace-server to get the same commands and behaviour to start and stop than the ones in the theia-trace-extension. Alternatively, you could implement your own vscode extension for that.

There are some APIs that the vscode-trace-extension that you could hook a trace server command to open a trace based on the file extension, see addTraceServerContributor in chapter https://github.com/eclipse-cdt-cloud/vscode-trace-extension?tab=readme-ov-file#using-the-external-api.

@GregSavin
Copy link
Author

Thanks! For now, I'm starting a trace server manually, and vscode-trace-extension seems to be working well, when opening traces using the file and folder dialog boxes.

However, when trying to open a trace programmatically from a test vscode extension, installing a test command like this:

context.subscriptions.push(vscode.commands.registerCommand('test.testcommand', () => {
	let uri = vscode.Uri.file('/tmp/my-trace-folder');
	if (uri) {
		vscode.commands.executeCommand("traces.openTraceFile", uri);
	}
}));

and then invoking my testcommand from the Command Palette, then I notice that sometimes, an editor pane opens with the Overview view present and populated with trace data, but the Trace Viewer pane still reflects "There are currently no open traces", but at that point if I close and re-open vscode, the Trace Viewer pane shows the trace that I opened programmatically in the previous session, and everything behaves well. But sometimes opening the same trace programmatically works as expected without restarting. I can try to characterize this observation more sharply (e.g. see if the overall behavior is deterministic when opening the same trace programmatically multiple times in sequence within a single vscode instance lifetime).

@jonahgraham
Copy link
Contributor

@bhufmann when you are back from vacation, I would like to get your opinion about this. Do you know that it may be useful in some cases for the OpenTraceWithPathCommand to return the newly opened TraceViewerWidget?

I don't have any use cases for that at the moment. It's not used in our user base. However, the OpenTraceWithPathCommand was contributed by @jonahgraham in commit 1c6babd and I'd like him to weigh in on not returning the TraceViewerWiget when executing this command. @jonahgraham would that be ok with you?

Our use case does not use the return value and I don't think that I coded it to return a value on purpose. I used the same style as the other similar commands and did not document a return value. HTH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants