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

ConnectedSocket DI scope for services running under WebSocketGateway #4287

Open
IMalyugin opened this issue Mar 12, 2020 · 7 comments
Open

Comments

@IMalyugin
Copy link

IMalyugin commented Mar 12, 2020

Feature Request

Add a ConnectedSocket scope for injectables.

Is your feature request related to a problem? Please describe.

There are currently 3 types of scopes:

  • default: useful for global request unaware services, that work in stateless manner;
  • request: super useful for handling request without having to pass context manually on each method call;
  • transient: useful for... well, using the same injection mechanics for in-place class instantiation;

That works well for normal http requests, but for websockets, request scope is not a thing according to docs:
image

WebSocket service design is built upon the idea that you have a "connection" which is a lot similar to a session or request, all the data transferred between connect and disconnect is usually treated as a single scope.

Out of all available mechanics to persist that data, we have @ConnectedSocket, which is only accessible from WebSocketGateway, so without a built in support for ConnectedSocket scope, we are forced to implement our own storages within socket and pass socket around in all the calls made between WebSocketGateway and services, which is exactly why Scopes.REQUEST was initially introduced in http.

Describe the solution you'd like

Add a new ConnectedSocket scope applicable to services that would create a new instance of a provider for each connection, within such services there is a need to access current socket client same way @Request decorator does for request-scope.

As far as I understand the problem lies within the fact that we must not generate new WebSocketGateway for connections, thus the propagation mechanics of Scopes break apart. But isn't @ConnectedSocket already enforcing similar breaches? As in, it is available within WebWocketGateway, but it is running within it's own virtual "scope".

Teachability, Documentation, Adoption, Migration Strategy

What is the motivation / use case for changing the behaviour?

Enforce similar system design on WebSockets as the already existing request within http eco-system to solve all the same troubles but within WebSocket context.

In addition, this would allow creating proper facades for Socket client to not call emit directly with no event/data type-checking.

@IMalyugin IMalyugin added needs triage This issue has not been looked into type: enhancement 🐺 labels Mar 12, 2020
@IMalyugin
Copy link
Author

IMalyugin commented Mar 12, 2020

To overcome current limitations for WebSocketGateway, I am forced to use my own "twisted dependency injection" mechanics, creating or passing instances of providers inside handleConnection, and performing cleanup on handleDisconnect.

This way if I want ConnectedSocket scoped providers - i instantiate them in handleConnection, while if i want to have a global injected providers, I provide them inside class constructor and pass down inside handleConnection.

image

@kamilmysliwiec kamilmysliwiec added scope: core scope: websockets and removed needs triage This issue has not been looked into labels Mar 13, 2020
@kamilmysliwiec kamilmysliwiec changed the title ConnectedSocket scope for services running under WebSocketGateway ConnectedSocket DI scope for services running under WebSocketGateway Mar 13, 2020
@pinstripe-potatohead
Copy link

This makes a lot of sense, and would be in line with the usual (connection scoped) socket.io handling

@flux627
Copy link

flux627 commented Jan 28, 2021

Would anyone here have advice as to how to make this workaround work when using GraphQL subscriptions? Does this use a WebsocketGateway under the hood? The subscriptions options object does expose onConnect and onDisconnect hooks, but it would seem that I can't inject dependencies in the context of those methods.

@IMalyugin
Copy link
Author

IMalyugin commented Jan 29, 2021

@flux627 I don't think there is a way to do this using standard Dependency Injection mechanics. The closest I could get (As described in the screenshot above) is the following:

  1. Injecting all the needed classes into your WebsocketGateway;
  2. Manually compile/cleanup the whole class structure inside onConnect/onDisconnect;

This approach adds extra code, but allows to utilize DI override mecanics.
(pagesService in example above is overridden using { useClass } from module)

As for "context", you can store everything inside websocketClient passed in as first argument to onConnect/onDisconnect. Everything attached to that client would be connection-based, you can also pass the client itself to other classes to provide access to that context to other classes.

I got to admit, it looks ugly, but it works.

@IMalyugin
Copy link
Author

IMalyugin commented Nov 10, 2021

Almost two years passed since the issue was created, I couldn't find the way back then and set the task aside. But now that I got back to it, I found a way to achieve the desired using WebsocketGateway and module refs.

Simply:

  1. Declare providers you wish to bind to connection as Request scoped;
  2. Generate contextId for each connection using ContextIdFactory;
  3. Create a custom provider - ConnectionGateway that resolves all the request-providers using moduleRef and stored contextId;
  4. Proxy all handlers in WebsocketGateway over to ConnectionGateway. This way you still use WebsocketGateway to access ws-context parameters, but when it comes to underlying services, you call ConnectionGateway, that incapsulated all the logic and provider interconnection.

Caution: WebsocketGateway should not use any connection-scoped providers, DI-logic starts from ConnectionGateway.


As for the original feature request, now that I had a little thought about it, I believe that instead of adding new ConnectedSocket DI scope for WebSocketGateway, the Request scope should be reused to match connection.

Having REQUEST scope binding to webSocket messages is a huge overhead, the typical websockets communication include hundreds of messages going in and out for every client.

The only problem is that it would introduce a breaking change, and there could be people out there already using message based REQUEST scope for web sockets.

@benripka
Copy link

benripka commented Nov 30, 2023

I think this would be a great feature... According to this comment it is not being pursued. Does anyone know why?

@IMalyugin
Copy link
Author

IMalyugin commented Nov 30, 2023

The problem is probably around a conflict between scopes and singularity of socket server. One of the requested feature - is to be able to inject the single server into other providers. Rewriting that in a simple provider mannor would require having a ubiquitous provider that is both available as singleton and request-scoped.

Not much can be done at this point, all the choices are heavy:

  • reverting to proper implementation right now would cause a critical breaking change;
  • adding a new scope, would require modifying framework core just for a single concrete purpose,

Both roads are a NO, for a stable framework. The only viable solution would be deprecating WebSocketGateway, and adding a new WebConnectionGateway. And even so, it's a pretty major feature for websockets, that aren't as popular of a feature to begin with.

Adding a WebConnectionGateway could probably be done in a community package. Or as an alternative, you can easily achieve the same result with a few lines of code (via moduleRef):

@Injectable()
export class ScopedConnectionInjector {
  constructor(private readonly moduleRef: ModuleRef) { }

  async register(socket: SocketIO.Socket) {
    (socket as any)[REQUEST_CONTEXT_ID] = ContextIdFactory.create();
    const connection = await this.getConnectionGateway(socket);
    connection.setConnection(socket);
  }

  async getConnectionGateway(socket: SocketIO.Socket) {
    const contextId = (socket as any)[REQUEST_CONTEXT_ID];
    return this.moduleRef.resolve(ConnectionGateway, contextId);
  }
}

Where ConnectionGateway is a simple request-scoped provider, that may in turn depend on other providers

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

No branches or pull requests

5 participants