1iveowl / WebsocketClientLite.PCL

Websocket client lite
MIT License
38 stars 14 forks source link

Feature request: Transfer ownership of TcpClient to WebsocketClientLite library #73

Closed ErisApps closed 2 years ago

ErisApps commented 2 years ago

Hi,

As the title already implied, I'd like to do a feature request regarding the TcpClient ownership handling in WebsocketClientLite. Currently, whenever one passes their own instance of TcpClient into the library, it will not get automatically disposed. However, I'd like to request a feature that does this regardless.

I currently pass my own instance into it in order to to create a proxied socket connection through Charles (an equivalent of Fiddler) so I can intercept all messages that are sent through the websocket. However, that results in me having to handle the disposing and closing of the websocket connection, which I honestly don't really know when to do properly, hence why transfering control to the library would solve that issue for me.

If anything is unclear or you would like to discuss this more, feel free to let me know. 🙂

1iveowl commented 2 years ago

Interesting use case.

I'd be happy to take a look at this, but please allow me to ask a question:

You write that you don't know when to dispose of the connected instance socket connection that you're passing to the WebSocket Client. Why not closing/disposing it at the same time as you dispose the WebSocket Client, as that is effectively what would happen if the feature request is implemented? Or am I missing something here?

In any case, a way to implement this would be to introduce a boolean valueHasTransferTcpSocketOwnership in this constructor:https://github.com/1iveowl/WebsocketClientLite.PCL/blob/95a76adfb6cc00684a908c010799676db19d06e0/src/main/WebsocketClientLite/MessageWebSocketRx.cs#L81

The HasTransferTcpSocketOwnership would by default be set to false but when set to true would always set the private property _keepTcpClientAlive to false : https://github.com/1iveowl/WebsocketClientLite.PCL/blob/95a76adfb6cc00684a908c010799676db19d06e0/src/main/WebsocketClientLite/Service/TcpConnectionService.cs#L39

ErisApps commented 2 years ago

I'd be happy to take a look at this, but please allow me to ask a question:

You write that you don't know when to dispose of the connected instance socket connection that you're passing to the WebSocket Client. Why not closing/disposing it at the same time as you dispose the WebSocket Client, as that is effectively what would happen if the feature request is implemented? Or am I missing something here?

Sure, I hope the following explanation will be clear enough, but if it isn't feel free to ask some follow-up questions. Anyways, in order to start the connection, we simply need to subscribe to the MessageWebsocketRx instance (but we can only do so once as subscribing multiple times will try to start the connection process multiple times) and to disconnect again, we dispose our subscription, kicking in the that logic in the library and doing the further clean-up/disposal of other relevant resources.

However, in some of my usecases, I need to trigger a restart of the websocket connection. So I call the disconnect logic right before executing the connect logic again.

https://github.com/ErisApps/CatCore/blob/2790ad640602ef96ed0aa069f93ef44afa24dcce/CatCore/Services/KittenWebSocketProvider.cs#L45-L138

Given that the disposal of the subscriptions is fire-n-forget, it's very much possible that the closing/disposing of the TcpClient happens before the connection is gracefully shut down, resulting in exceptions being thrown from within the library. (which is the main reason why I created this issue in the first place 🙂 )

As for your proposed solution, that was very much the idea that I had in mind as well so that's completely fine for me.

1iveowl commented 2 years ago

Thanks. Makes sense.

Implementing the transfer of ownership if the socket this way is fairly trivial, and it doesn't break anything as far as I can tell. I'll have look at it.

1iveowl commented 2 years ago

Implemented in release 7.3.