eclipse-cdt-cloud / theia-trace-extension

Eclipse Theia trace viewer extension using the Trace Server Protocol (TSP), through the tsp-typescript-client. Also the home for reusable JavaScript libraries: traceviewer-base, traceviewer-react-components
MIT License
49 stars 60 forks source link

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

Open GregSavin opened 1 month ago

GregSavin commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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

marcdumais-work commented 1 month ago

=> Found "msgpackr@1.6.1"

The bug I remembered: https://github.com/eclipse-theia/theia/issues/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: https://github.com/eclipse-cdt-cloud/theia-trace-extension/blob/d7a82f3bfafd4738d1168fdff3687dd9a136e41a/package.json#L49-L50

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

GregSavin commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 3 weeks ago

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

bhufmann commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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