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

Fix ws/wss tcp socket leaks, closes #162 #163

Closed
wants to merge 1 commit into from

Conversation

xadhoom
Copy link

@xadhoom xadhoom commented Jul 21, 2022

Since sofia doesn't have a proper http client, outgoing connections with ws/wss transport are not possible.

Using the default tport_base_connect will lead into socket leaks, because they'll get created but never freed.

Instead just define the proper vtp_connect function in ws/wss tranport which will always return NULL and make outgoing ws/wss connections to fail correctly.

@freeswitch-ci-system
Copy link

Since sofia doesn't have a proper http client, outgoing connections
with ws/wss transport are not possible. Using the default
tport_base_connect will lead into socket leaks, because they'll get
created but never freed. Instead just define the proper vtp_connect
function in ws/wss tranport which will always return NULL and make
outgoing ws/wss connections to fail correctly.
@xadhoom xadhoom force-pushed the fix_ws_socket_leak branch from adc2ecf to b90274e Compare July 21, 2022 13:06
@freeswitch-ci-system
Copy link

@xadhoom
Copy link
Author

xadhoom commented Sep 30, 2022

No feedback here?

I want to give a feedback from us with a graph that speaks by itself
chart

(The scale is written wrongly M really means GB)

This is the memory usage of FS in a production system where websockets (plain ones, not ssl) are heavily used and a lot of disconnection may happen. Those connections are pinged regularly for reachability using sofia options.

Around 22 July is where the final reboot with the above patch, see the memory graph? Speaks by itself. No other changes has been added since and traffic patterns have not changed.

@andywolk
Copy link
Member

andywolk commented Oct 3, 2022

Would be nice to have a way to reproduce that.

@xadhoom
Copy link
Author

xadhoom commented Oct 11, 2022

Hi,

there're two points here: first a logic error, since no connect callbacks are defined in sofia ws/wss transport the default tport_base_connect is used, which is not appropriate since has nothing to do with websockets and is not handled correctly (leading to socket leaks).
So, just fixing this logic error imho is a motivation to include the patch or at least a proper fix.

Then, to simulate the leak the question is on how to force sofia ws/wss transport to do outgoing connections (which results into calling the default tport_base_connect which then create the leak because not cleared by the transport itselg).

We reproduce this by:

  • set freeswitch to send option pings
  • set freeswitch to not de-register the client if tcp connection closes
  • abruptly interrupt the tcp ws/wss connection, in order to not clear the reg (this happens in real world scenarios where devices disappears from the network for many reasons)
  • from now fs tries to send options to the ws/wss client (because even if unreach is still registered)
  • wait for the leak, the rate depends on how many clients do you have and how often the option ping is done

@robertoscarpone
Copy link

Hi, I’m curious, do you use WS or WSS in your environment?

@xadhoom
Copy link
Author

xadhoom commented Feb 9, 2023

Hi, I’m curious, do you use WS or WSS in your environment?

Right now WS, proxied by nginx for SSL. WSS has some serious issues in corrupting stuff, like FS sqlite databases... go figure.

Anyway, this patch is not related to WS or WSS, it applies to both.

@robertoscarpone
Copy link

Thank you for the reply!

Anyway, this patch is not related to WS or WSS, it applies to both.

Yes, I asked you because we are struggling with SSL handling in sofia's WSS implementation.

Right now WS, proxied by nginx for SSL.

We would also like to use WS with nginx, but since it doesn't handle passing the clients IP, we can't do this right now.

@andywolk
Copy link
Member

andywolk commented Nov 7, 2023

Fixed by #233

@andywolk andywolk closed this Nov 7, 2023
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 this pull request may close these issues.

3 participants