Fix "Converting circular structure to JSON" Error #5121

vinokurig · 2019-05-11T12:27:50Z

Fix "Converting circular structure to JSON" Error that causes when launching VsCode GitHub-Pull-Request-Plugin.
Is needed for eclipse-che/che#11867

akosyakov · 2019-05-13T08:03:48Z

packages/plugin-ext/src/api/rpc-protocol.ts

+                try {
+                    return JSON.parse(JSON.stringify(value));
+                } catch (e) {
+                    return;


VS Code translates circular ref to null

changed to return undefined

akosyakov · 2019-05-13T08:03:56Z

@vinokurig How can one verify it? Is it about: #4975?

vinokurig · 2019-05-13T08:23:55Z

@akosyakov looks like it should solve the issue, I'll check it

packages/plugin-ext/src/api/rpc-protocol.ts

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2019-05-13T11:43:14Z

@akosyakov I've launched the microclimate-vscode plugin (#4975) and the issue was not reproduced:

packages/plugin-ext/src/api/rpc-protocol.ts

akosyakov · 2019-05-13T12:15:49Z

Why cannot we do it the same like VS Code do: https://github.com/microsoft/vscode/blob/0fde6619172c9f04c41f2e816479e432cc974b8b/src/vs/workbench/services/extensions/common/rpcProtocol.ts#L22-L28

Without caches and serializing/deserializing all objects (which look expensive)? It seems to be more performant, compatible with VS Code and simple. Plus apply it in all places, not only for replyOk?

vinokurig · 2019-05-14T08:50:10Z

@akosyakov The plugin didn't work with that solution. Probably VsCode uses another replacers.

akosyakov · 2019-05-15T05:58:29Z

packages/plugin-ext/src/api/rpc-protocol.ts

+                    return undefined;
+                }
+            }
+            cache.push(value);


How does it ever reach this code?

akosyakov · 2019-05-15T06:00:45Z

@vinokurig Don't you think it is bad to serilaize and parse each property of each object? I don't think this solution is in the right direction. VS Code replacer only handles URI i wonder what we do differently that we have to go for such expensive strategies.

benoitf · 2019-05-15T07:51:20Z

I also agree that the fix should be better than what is being done. It's too costly

benoitf

somehow the fix should avoid to unwrap/wrap each element

…\n Signed-off-by: Ariel Bentolila <ariel.bentolila@sap.com>

ariel-bentu · 2019-05-18T19:48:54Z

Hi,
as you can see I proposed a PR to fix this, I was circling the same problem and then I saw this thread.
Please consider this one. It solved my issue, maybe will solve also the ones mentioned in this thread?
Thanks

#5185

…\n Signed-off-by: Ariel Bentolila <ariel.bentolila@sap.com> Signed-off-by: Ariel Bentolila <ariel.bentolila@sap.com>

ariel-bentu · 2019-05-20T13:14:21Z

After some more investigation and the latest PR I made #5196, I think I figured out what is fundamentally wrong in tree views for plugin/vscde-extensions in Theia, which exposes this issue in many cases:

The way the tree view is implemented (TreeViewsExt, TreeViewItem, tree-view-main.tsx etc) is that the server-side object representing a TreeItem is moved (via json-rpc) from server to client upon getChildren call (specifically the original object is placed under metadata defined in TreeViewItem.
Then, upon context-menu, or inline-action-icons the relevant command is triggered from the browser via rpc passing back the object stored in the member metadata, to the command.

This back and forth move of the actual object of the tree-item is the source of some major problems:

these objects tend to have circular-references, thus pose a challenge to serialize them (fixed in my PR)
These objects could be large, and serve no purpose on the client other than to be sent back in case of menu/action-icon clicked

When they are deserialized back to object on the server they loose all the functions and private data they had before sent to the browser - thus break a lot of existing vscode-extensions.

The right way to do it (and I guess vscode do exactly that), is to avoid sending these objects to the browser. Upon menu or click on action icon, send to the server a call indicating that the menu was triggered, or the action-icon was clicked. the actual for the command behind the menu or action icon would be called from the server-side, passing the original item's object (with all its functions and private data).

What do you think?

I believe this would solve #4975 as well.

benoitf · 2019-05-20T13:48:20Z

@ariel-bentu 👍
it would be definitely a better fix if no circular JSON is sent (and that we don't send large data over the network for nothing)

ariel-bentu · 2019-05-21T07:21:56Z

I updated PR #5196 to do that. please help to promote its acceptance.

vinokurig · 2019-07-04T07:38:28Z

It is actual since this was added: https://github.com/theia-ide/theia/blob/16a54cc8579569cbcb3c8ea02b2125eceeaa4633/packages/plugin-ext/src/plugin/tree/tree-views.ts#L255

vinokurig · 2019-07-05T07:26:02Z

Will fix in another way

vinokurig requested review from benoitf, evidolob, azatsarynnyy and akosyakov May 11, 2019 12:27

vinokurig force-pushed the fixRpcError branch from 50036a2 to 036cafa Compare May 11, 2019 12:28

akosyakov reviewed May 13, 2019

View reviewed changes

kittaakos reviewed May 13, 2019

View reviewed changes

packages/plugin-ext/src/api/rpc-protocol.ts Outdated Show resolved Hide resolved

vinokurig force-pushed the fixRpcError branch 2 times, most recently from 1b81e92 to a5e02f0 Compare May 13, 2019 11:36

Fix "Converting circular structure to JSON" Error

f381bfc

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig force-pushed the fixRpcError branch from a5e02f0 to f381bfc Compare May 13, 2019 11:38

kittaakos reviewed May 13, 2019

View reviewed changes

packages/plugin-ext/src/api/rpc-protocol.ts Show resolved Hide resolved

akosyakov reviewed May 15, 2019

View reviewed changes

packages/plugin-ext/src/api/rpc-protocol.ts

return undefined;

}

}

cache.push(value);

Copy link

Member

akosyakov May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it ever reach this code?

vparfonov mentioned this pull request May 15, 2019

IDE-2019-06-12 (Sprint: 167) eclipse-che/che#13343

Closed

15 tasks

benoitf requested changes May 17, 2019

View reviewed changes

ariel-bentu pushed a commit to ariel-bentu/theia that referenced this pull request May 18, 2019

fixes eclipse-theia#5121 - circular references in JSON in RPC traffic…

30d9d2c

…\n Signed-off-by: Ariel Bentolila <ariel.bentolila@sap.com>

ariel-bentu pushed a commit to ariel-bentu/theia that referenced this pull request May 18, 2019

fixes eclipse-theia#5121 - circular references in JSON in RPC traffic…

02b71cd

…\n Signed-off-by: Ariel Bentolila <ariel.bentolila@sap.com>

ariel-bentu mentioned this pull request May 18, 2019

Jsoncycles #5185

Closed

ariel-bentu mentioned this pull request May 20, 2019

handle cycles in json.stringify #5196

Closed

akosyakov mentioned this pull request May 30, 2019

[plugin/tree] don't pass tree item data over json-rpc #5310

Merged

akosyakov closed this in #5310 Jun 1, 2019

vinokurig reopened this Jul 4, 2019

vinokurig closed this Jul 5, 2019

vinokurig deleted the fixRpcError branch August 5, 2019 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Converting circular structure to JSON" Error #5121

Fix "Converting circular structure to JSON" Error #5121

vinokurig commented May 11, 2019

akosyakov May 13, 2019

vinokurig May 13, 2019

akosyakov commented May 13, 2019

vinokurig commented May 13, 2019

vinokurig commented May 13, 2019 •

edited

Loading

akosyakov commented May 13, 2019 •

edited

Loading

vinokurig commented May 14, 2019

akosyakov May 15, 2019

akosyakov commented May 15, 2019

benoitf commented May 15, 2019

benoitf left a comment

ariel-bentu commented May 18, 2019

ariel-bentu commented May 20, 2019

benoitf commented May 20, 2019

ariel-bentu commented May 21, 2019

vinokurig commented Jul 4, 2019

vinokurig commented Jul 5, 2019

Fix "Converting circular structure to JSON" Error #5121

Fix "Converting circular structure to JSON" Error #5121

Conversation

vinokurig commented May 11, 2019

akosyakov May 13, 2019

Choose a reason for hiding this comment

vinokurig May 13, 2019

Choose a reason for hiding this comment

akosyakov commented May 13, 2019

vinokurig commented May 13, 2019

vinokurig commented May 13, 2019 • edited Loading

akosyakov commented May 13, 2019 • edited Loading

vinokurig commented May 14, 2019

akosyakov May 15, 2019

Choose a reason for hiding this comment

akosyakov commented May 15, 2019

benoitf commented May 15, 2019

benoitf left a comment

Choose a reason for hiding this comment

ariel-bentu commented May 18, 2019

ariel-bentu commented May 20, 2019

benoitf commented May 20, 2019

ariel-bentu commented May 21, 2019

vinokurig commented Jul 4, 2019

vinokurig commented Jul 5, 2019

vinokurig commented May 13, 2019 •

edited

Loading

akosyakov commented May 13, 2019 •

edited

Loading