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.

Sign up for GitHub

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

Jump to bottom

New IPC does not handle debugging gracefully #11203

Closed
colin-grant-work opened this issue May 25, 2022 · 10 comments · Fixed by #11228
Closed

New IPC does not handle debugging gracefully #11203

colin-grant-work opened this issue May 25, 2022 · 10 comments · Fixed by #11228
Labels
bug bugs found in the application

Comments

@colin-grant-work
Copy link
Contributor

colin-grant-work commented May 25, 2022 •
edited
Loading

Bug Description:

When debugging in the browser and after sitting on a breakpoint for some time, resuming the application produces tens of thousands of logs and crashes the browser.

Steps to Reproduce:

  1. In the browser application (may also apply to Electron)
  2. Set a break point in the developer tools and hit it.
  3. Wait a few minutes.
  4. Remove your breakpoint and hit continue.
  5. Observe tens of thousands of logs about failure to send a message. I took the screenshot below a few seconds into the process - eventually the counter got over 250,000 before the browser killed the page as unresponsive.

@tortmayr, @JonasHelming, @tsmaeder, @paul-marechal

image

Additional Information

  • Operating System: OS X, Chrome 101
  • Theia Version: 9a10b6b
@colin-grant-work colin-grant-work added the bug bugs found in the application label May 25, 2022
@colin-grant-work
Copy link
Contributor Author

For me, this is a showstopper - if we don't fix this, we shouldn't release with the new IPC.

@JonasHelming
Copy link
Contributor

@colin-grant-work : Just to be sure I get this right: You run a Theia-based application (e.g. the example application) and then you set a break point in this Theia-based application using the developer tools, correct?

@JonasHelming
Copy link
Contributor

@colin-grant-work @paul-marechal @vince-fugnitto : Tobi can have a look at this asap. However, tomorrow is a public holiday for us and we won't be in office. So we can either revert the change for now or delay the release. As this does not look completely trivial, we obviously cannot estimate when it can be fixed, but we can start to look at it with high prio on Friday.

Opinions?

@colin-grant-work
Copy link
Contributor Author

@colin-grant-work : Just to be sure I get this right: You run a Theia-based application (e.g. the example application) and then you set a break point in this Theia-based application using the developer tools, correct?

Exactly. This was in the example application on master in the browser, and I set a breakpoint (@theia/monaco-editor-core/src/vs/editor/common/languages.ts:143, though I don't think that particular detail matters), browsed some source code for a while, and when I resumed the application after removing my breakpoint, the console flooded and it crashed. I've reproduced it a couple of times.

@JonasHelming
Copy link
Contributor

@colin-grant-work : Just to be sure I get this right: You run a Theia-based application (e.g. the example application) and then you set a break point in this Theia-based application using the developer tools, correct?

Exactly. This was in the example application on master in the browser, and I set a breakpoint (@theia/monaco-editor-core/src/vs/editor/common/languages.ts:143, though I don't think that particular detail matters), browsed some source code for a while, and when I resumed the application after removing my breakpoint, the console flooded and it crashed. I've reproduced it a couple of times.

Might be even related to #11199

@vince-fugnitto
Copy link
Member

@JonasHelming given how there were already three issues identified after the merge and potentially others perhaps we should revert. The issue with postponing the release is that we cannot identify how long it will take to resolve the bugs and I wouldn’t want to postpone indefinitely. It would have probably been safer overall to have waited for Friday to merge in any case.

@JonasHelming
Copy link
Contributor

@vince-fugnitto : I agree and I was actually originally in favor of waiting until Friday. May I ask you or somebody else to do the revert, please? I am traveling without my PC and I do not want to try a revert on my mobile.

@JonasHelming
Copy link
Contributor

... and sorry for the trouble

@colin-grant-work colin-grant-work mentioned this issue May 25, 2022
Merged
1 task
@colin-grant-work
Copy link
Contributor Author

... and sorry for the trouble

Not at all. I'm sure it will be a great improvement to the framework, once all the kinks are worked out. Just good to have some time between the merge and the next release to stress test it under a variety of real-world conditions.

tortmayr added a commit to eclipsesource/theia that referenced this issue May 31, 2022
@tortmayr
Fix critical bugs
ea5e5a4 
- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics
@tortmayr tortmayr mentioned this issue May 31, 2022
Closed
1 task
tortmayr added a commit to eclipsesource/theia that referenced this issue May 31, 2022
@tortmayr
Fix critical bugs
562f3ed 
- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics
@tortmayr tortmayr mentioned this issue May 31, 2022
Merged
1 task
tortmayr added a commit to eclipsesource/theia that referenced this issue May 31, 2022
@tortmayr
Fix critical bugs
53262af 
- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics
@tortmayr
Copy link
Contributor

@colin-grant-work Thank you for catching these critical bugs and taking care of the revert. I have now openen a new PR that reapplies the new message-rpc framework and fixes the identfied issues: eclipsesource#40.

tortmayr added a commit to eclipsesource/theia that referenced this issue May 31, 2022
@tortmayr
Fix critical bugs
3af21f9 
- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics
@colin-grant-work colin-grant-work linked a pull request May 31, 2022 that will close this issue
Reapply changes for new message rpc #11228
Merged
1 task
tortmayr added a commit to eclipsesource/theia that referenced this issue Jun 1, 2022
@tortmayr @tsmaeder
Integrate new message-rpc prototype into core messaging API (extensio…
e3cd038 
…ns) (eclipse-theia#11011)

Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also eclipse-theia#10781).
The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data.

Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc).
This has major impacts on the Messaging API as we no longer expose a  `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead.

- Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc)
- Replace usage of  `vscode-ws-jsonrpc` with a custom binary RPC protocol.
- Remove 'vscode-ws-jsonrpc' depdency from "@theia/core/shared".
- Refactor all connection providers to use the new binary protocol.
- Ensure that the `RemoteFileSystemProvider` API uses  `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead  of unnecessarily converting from and to `Uint8Arrays`.
- Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol.
- Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding.
- Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`.

The plugin API still uses its own RPC protocol implementation. This will be addressed in a follow-up PR. (See eclipse-theia#11093)

Fix critical bugs

- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics.

Co-authored-by: Thomas Mäder <tmader@redhat.com>
tortmayr added a commit to eclipsesource/theia that referenced this issue Jun 1, 2022
@tortmayr @tsmaeder
Integrate new message-rpc prototype into core messaging API (extensio…
d48c496 
…ns) (eclipse-theia#11011)

Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also eclipse-theia#10781).
The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data.

Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc).
This has major impacts on the Messaging API as we no longer expose a  `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead.

- Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc)
- Replace usage of  `vscode-ws-jsonrpc` with a custom binary RPC protocol.
- Remove 'vscode-ws-jsonrpc' depdency from "@theia/core/shared".
- Refactor all connection providers to use the new binary protocol.
- Ensure that the `RemoteFileSystemProvider` API uses  `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead  of unnecessarily converting from and to `Uint8Arrays`.
- Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol.
- Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding.
- Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`.

The plugin API still uses its own RPC protocol implementation. This will be addressed in a follow-up PR. (See eclipse-theia#11093)

Fix critical bugs

- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics.

Co-authored-by: Thomas Mäder <tmader@redhat.com>
tortmayr added a commit to eclipsesource/theia that referenced this issue Jun 2, 2022
@tortmayr @tsmaeder
Integrate new message-rpc prototype into core messaging API (extensio…
982dda7 
…ns) (eclipse-theia#11011)

Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also eclipse-theia#10781).
The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data.

Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc).
This has major impacts on the Messaging API as we no longer expose a  `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead.

- Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc)
- Replace usage of  `vscode-ws-jsonrpc` with a custom binary RPC protocol.
- Remove 'vscode-ws-jsonrpc' depdency from "@theia/core/shared".
- Refactor all connection providers to use the new binary protocol.
- Ensure that the `RemoteFileSystemProvider` API uses  `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead  of unnecessarily converting from and to `Uint8Arrays`.
- Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol.
- Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding.
- Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`.

The plugin API still uses its own RPC protocol implementation. This will be addressed in a follow-up PR. (See eclipse-theia#11093)

Fix critical bugs

- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics.

Co-authored-by: Thomas Mäder <tmader@redhat.com>
tortmayr added a commit to eclipsesource/theia that referenced this issue Jun 2, 2022
@tortmayr @tsmaeder
Integrate new message-rpc prototype into core messaging API (extensio…
12ce278 
…ns) (eclipse-theia#11011)

Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also eclipse-theia#10781).
The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data.

Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc).
This has major impacts on the Messaging API as we no longer expose a  `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead.

- Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc)
- Replace usage of  `vscode-ws-jsonrpc` with a custom binary RPC protocol.
- Remove 'vscode-ws-jsonrpc' depdency from "@theia/core/shared".
- Refactor all connection providers to use the new binary protocol.
- Ensure that the `RemoteFileSystemProvider` API uses  `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead  of unnecessarily converting from and to `Uint8Arrays`.
- Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol.
- Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding.
- Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`.

The plugin API still uses its own RPC protocol implementation. This will be addressed in a follow-up PR. (See eclipse-theia#11093)

Fix critical bugs

- eclipse-theiaGH-11196 Remove dev/debug logoutput from IPCChannel
- eclipse-theiaGH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- eclipse-theia#11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for eclipse-theia#11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes eclipse-theia#11196
Fixes eclipse-theia#11199

Contributed on behalf of STMicroelectronics.

Co-authored-by: Thomas Mäder <tmader@redhat.com>
JonasHelming pushed a commit that referenced this issue Jun 2, 2022
@tortmayr @tsmaeder
Integrate new message-rpc prototype into core messaging API (extensio…
83d4308 
…ns) (#11011) (#11228)

Refactors and improves the prototype of a faster JSON-RPC protocol initially contributed by @tsmaeder (See also #10781).
The encoding approach used in the initial POC has some performance drawbacks when encoding plain JSON objects. We refactored the protocol to improve the performance for JSON objects whilst maintaining the excellent performance for encoding objects that contain binary data.

Integrates the new message-rpc prototype into the core messaging API (replacing vscode-ws-jsonrpc).
This has major impacts on the Messaging API as we no longer expose a  `Connection` object (which was provided by vscode-ws-jsonrpc) and directly rely on a generic transport `Channel` implementation instead.

- Introduce `Channel` as the main transport concept for messages (instead of the dedicated `Connection` from vscode-jsonrpc)
- Replace usage of  `vscode-ws-jsonrpc` with a custom binary RPC protocol.
- Remove 'vscode-ws-jsonrpc' depdency from "@theia/core/shared".
- Refactor all connection providers to use the new binary protocol.
- Ensure that the `RemoteFileSystemProvider` API uses  `Uint8Arrays` over plain number arrays. This enables direct serialization as buffers and reduces the overhead  of unnecessarily converting from and to `Uint8Arrays`.
- Refactor terminal widget and terminal backend contribution so that the widgets communicates with the underlying terminal process using the new rpc protocol.
- Rework the IPC bootstrap protocol so that it uses a binary pipe for message transmission instead of the `ipc` pipe which only supports string encoding.
- Extend the `JsonRpcProxyFactory` with an optional `RpcConnectionFactory` that enables adopter to creates proxies with a that use a custom `RpcProtocol`/`RpcConnection`.

The plugin API still uses its own RPC protocol implementation. This will be addressed in a follow-up PR. (See #11093)

Fix critical bugs

- GH-11196 Remove dev/debug logoutput from IPCChannel
- GH-11199 Refactor connection provider and channel multiplexer to properly handle a reconnecting backend
   -  Remove console log in websocket-channel if the socket is not connected. Otherwise we end up in an endless loop.
   -  Ensure that channels & RpcProtocol instances proplery dispose all resources if the backend disconnects.
   -  Ensure that all previously open channels and RpcProtocol instances are properly restored once the backend reconnects.

- #11203 Ensure that debugging is handled gracefully (implicitly fixed with the fix for #11199)

- Remove dependency to `reconnecting-websocket` which is no longer needed since the swap to socket.io

Fixes #11196
Fixes #11199

Contributed on behalf of STMicroelectronics.

Co-authored-by: Thomas Mäder <tmader@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug bugs found in the application
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reapply changes for new message rpc eclipsesource/theia
4 participants
@JonasHelming @tortmayr @vince-fugnitto @colin-grant-work