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

Theia creates 2 web-sockets if we override the WebSocketConnectionProvider class #13094

Closed
safisa opened this issue Nov 23, 2023 · 17 comments · Fixed by #13199
Closed

Theia creates 2 web-sockets if we override the WebSocketConnectionProvider class #13094

safisa opened this issue Nov 23, 2023 · 17 comments · Fixed by #13199

Comments

@safisa
Copy link
Contributor

safisa commented Nov 23, 2023

Bug Description:

Hi,
It could be difficult to explain the issue I am facing on the latest Theia (1.43.1), but I will try:

I have a class that extends the WebSocketConnectionProvider:

export class CustomWebSocketConnectionProvider extends WebSocketConnectionProvider {
    protected override createWebSocket(url: string): Socket {
        ...
    }
}

And here is the binding code:

bind(CustomWebSocketConnectionProvider).toSelf().inSingletonScope();
rebind(WebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);

It was working correctly until I upgraded to 1.43.1 (from 1.40). I see now in the dev-tools on the network tab that there are two web sockets, and only one is actually working (before that there was only one).

If I debug the code from dev-tools and put a breakpoint inside the createWebSocket method in both classes (CustomWebSocketConnectionProvider and the base WebSocketConnectionProvider class), and reload the page, the break point breaks in both of them! this should not be the case, since I override this class and rebind it to the new one.

I think this issue is caused by the Remote SSH PR from 1.43: #12618

Steps to Reproduce:

  1. create a new class that extends the WebSocketConnectionProvider
  2. it can be an empty class, no need to override anything
  3. add the binding as above
  4. reload and check the network tab with WS filtering
  5. you will see 2 WS
  6. also you can override the createWebSocket, you can just call the super once, and put the breakpoint as described above..

Additional Information

  • Operating System: Mac
  • Theia Version: 1.43.1
@msujew
Copy link
Member

msujew commented Nov 23, 2023

@safisa You probably need to rebind the local websocket connection provider then as well:

rebind(LocalWebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);

See also our own messaging module.

@safisa
Copy link
Contributor Author

safisa commented Nov 23, 2023

@msujew I already tried this but didn't help, same issue.

@safisa
Copy link
Contributor Author

safisa commented Nov 23, 2023

@msujew I think it is related to the PR from 1.42 and not the one above: 12590

But, I don't know how to make my custom binding work at the preload level.
I tried to move my binding from my frontend module to a new preload module like this:

src/browser/preload/custom-preload-module.ts:

export default new ContainerModule((bind, unbind, isBound, rebind) => {
    bind(CustomWebSocketConnectionProvider).toSelf().inSingletonScope();
    rebind(WebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);
    //rebind(LocalWebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);
});

package.json:

"theiaExtensions": [
    {
      "frontendPreload": "lib/browser/preload/custom-preload-module"
    },
    {....}

but this also not working, the custom preload file is not loaded at all. Maybe this is not how we can contribute to preload step!

Can you help?

Thanks

@msujew
Copy link
Member

msujew commented Nov 23, 2023

Right, it's likely more related to #12590

but this also not working, the custom preload file is not loaded at all. Maybe this is not how we can contribute to preload step!

It should work though. Can you see whether your frontendPreload gets generated to the preload method in src-gen/frontend/index.js of your Theia app?

Note that you need to rebind the WebSocketConnectionProvider both on frontend and frontendPreload level.

@safisa
Copy link
Contributor Author

safisa commented Nov 23, 2023

@msujew
Finally, it is working :)
First I was in watch mode, thus the index.js was not updated, I had to rebuild it using yarn, and then restart my app.
Also, I had to do the binding as you said in the frontend 👍

Thanks a lot.

@msujew
Copy link
Member

msujew commented Nov 23, 2023

Alright, that's good to know. Closing the issue :)

@msujew msujew closed this as completed Nov 23, 2023
@safisa
Copy link
Contributor Author

safisa commented Dec 11, 2023

Hi @msujew

I see that there is an error message on the browser console log regarding the rebind above:

index.js:48 Failed to run preload scripts.

index.js:50 Error: Could not unbind serviceIdentifier: WebSocketConnectionProvider
    at Container._removeServiceFromDictionary (container.ts:699:13)
    at Container.unbind (container.ts:201:10)
    at Container.rebind (container.ts:184:10)
    at container.ts:526:38
    at ContainerModule.registry (custom-preload-module.ts:11:5)
    at Container.load (container.ts:119:21)
    at index.js:35:1
    at async preload (index.js:43:1)
    at async index.js:59:1

My custom class works as expected, but does the error above affect anything?

I tried to do just bind not rebind in the preload module, in this case, there is no error message in the log, but I see 2 web socket connections in the network (dev tools), one is active (all the messages go through it) and the other one has several messages only...

Thanks

@msujew
Copy link
Member

msujew commented Dec 11, 2023

My custom class works as expected, but does the error above affect anything?

Yes, it will prevent all preload scripts from running (as indicated in the error message), i.e. identifying backend OS and loading translations.

Instead of calling rebind it might be enough to just call bind. Since all rebind does is call unbind and then bind.

@safisa
Copy link
Contributor Author

safisa commented Dec 11, 2023

@msujew but once I tried to do only bind, I see 2 web sockets in the network tab in dev tools:

image

In the image above the selected one is the inactive one.
So why we do now have 2 sockets instead of one?

@msujew msujew reopened this Dec 14, 2023
@hyddel
Copy link

hyddel commented Dec 14, 2023

@msujew as mentioned here, localizations are still not loading.

I have created a preload-module where I have only one customization which is websocket one:

bind(CustomWebSocketConnectionProvider).toSelf().inSingletonScope();
rebind(WebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);

so here is my setting:

"theiaExtensions": [
    {
      "frontendPreload": "lib/browser/preload/preload-module-ws"
    },
    {
      "frontend": "lib/browser/<our-custom-module>",
      "backend": "lib/node/<our-custom-module>"
    }
  ]

The frontend is pointing to all other customizations. Even then, language translations are not loading.

In addition, I see errors in the console logs:

Failed to run preload scripts.

Error: Could not unbind serviceIdentifier: n
    _removeServiceFromDictionary container.ts:699
    unbind container.ts:201
    rebind container.ts:184
    qe container.ts:526
    default preload-module.ts:8
    load container.ts:119
    r index.js:43
    promise callback*r index.js:43
    85340 index.js:51
    exports index.js:67
    85340 index.js:63
    i main.js:2222
    <anonymous> main.js:2222
    <anonymous> main.js:2222

@hyddel
Copy link

hyddel commented Dec 15, 2023

@msujew any thoughts ?

@hyddel
Copy link

hyddel commented Dec 18, 2023

@msujew I have tried the following in the preload module after which localizations were loading but websocket connection was not getting established.

bind(CustomWebSocketConnectionProvider).toSelf().inSingletonScope();
bind(WebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);

One thing to mention here is that we have the following code in our custom-frontend-application-module where we have all rebindings and this module is pointed by the frontend mentioned in the package.json file

NOTE - WebSocketConnectionProvider is getting rebinded twice. One occurrence is in the preload module and other one is in the frontend module

bind(CustomWebSocketConnectionProvider).toSelf().inSingletonScope();
rebind(WebSocketConnectionProvider).toService(CustomWebSocketConnectionProvider);

bind(CustomPluginViewRegistry).toSelf().inSingletonScope();
rebind(PluginViewRegistry).toService(CustomPluginViewRegistry);

bind(CustomWorkspaceService).toSelf().inSingletonScope();
rebind(WorkspaceService).toService(CustomWorkspaceService);

bind(CustomFrontendApplicationContribution).toSelf().inSingletonScope();
bind(FrontendApplicationContribution).to(CustomFrontendApplicationContribution).inSingletonScope();

bind(CustomCommonFrontendContribution).toSelf().inSingletonScope();
rebind(CommonFrontendContribution).toService(CustomCommonFrontendContribution);

@hyddel
Copy link

hyddel commented Dec 19, 2023

@msujew could you please try rebind something(preferrably websocket connection provider) in frontendPreload ? I think frontendPreload is not able to unbind and rebind. Without unbinding and rebinding, custom WS connection providers cannot be loaded.

@msujew
Copy link
Member

msujew commented Dec 20, 2023

@hyddel @safisa I was able to reproduce this and fixed the issue in #13199.

@hyddel
Copy link

hyddel commented Dec 20, 2023

@msujew Thank you for fixing the issue. I have added your fix manually on my local machine after which I am seeing a new error in the console logs. Not sure if it is some kind of regression or I am missing something.

My environement:

  • operating system - Mac
  • Theia version - 1.42.1 (with the above fix, I have added your fix manually)

Here is the code to reproduce it:

custom-code-editor.tar.gz

From the root of the above directory, run the following command:

yarn && cd browser-app && yarn download:plugins && yarn local_start

and open "http://localhost:4444/". In the console logs, you will see the below error:

Failed to start the frontend application.

Error: Ambiguous match found for serviceIdentifier: a
    re planner.js:69
    re planner.js:45
    se planner.js:80
    i planner.js:131
    qe container.js:604
    _get container.js:574
    _getButThrowIfAsync container.js:577
    get container.js:352
    toService binding_to_syntax.js:80
    Ce resolver.js:72
    Ce resolver.js:72
    _e resolver.js:90
    Se resolver.js:105
    we resolver.js:99
    Se resolver.js:104
    ye resolver.js:66
    qe resolver.js:203
    qe container.js:606
    _get container.js:574
    _getButThrowIfAsync container.js:577
    get container.js:352
    createProxy ws-connection-provider.ts:48
    frontendApplicationModule frontend-application-module.ts:250
    ke resolver.js:138
    Ee resolver.js:116
    Se resolver.js:110

@msujew
Copy link
Member

msujew commented Dec 20, 2023

@hyddel Since we now use the same DI container for both the preload and the runtime of the app, you're not allowed to call bind(CustomWebSocketConnectionProvider) twice. Rebinding the WebSocketConnectionProvider once (during the preload) is enough.

@hyddel
Copy link

hyddel commented Dec 20, 2023

got it, thanks.

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

Successfully merging a pull request may close this issue.

3 participants