Skip to content

Fix WebSocket connections leak#3599

Closed
lucalooz wants to merge 1 commit intoSignalR:masterfrom
lucalooz:master
Closed

Fix WebSocket connections leak#3599
lucalooz wants to merge 1 commit intoSignalR:masterfrom
lucalooz:master

Conversation

@lucalooz
Copy link
Copy Markdown

Errors occurring during the initialization of a WebSocket weren’t
notified to the WebSocketTransport that was leaking by returning
IsAlive = true forever

Errors occurring during the initialization of a WebSocket weren’t
notified to the WebSocketTransport that was leaking by returning
IsAlive = true forever
@dnfclas
Copy link
Copy Markdown

dnfclas commented Nov 23, 2015

Hi @Idhor, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

@davidfowl
Copy link
Copy Markdown
Member

When does this happen?

@lucalooz
Copy link
Copy Markdown
Author

lucalooz commented Dec 2, 2015

onClose/onError listeners aren't set immediately when using the websocket so if an error occurs during the initialization the transport will be considered alive forever even if the real websocket is closed and disposed
I had this issue with 50 connections leak per day
If i'm not wrong this is already fixed on signalr for asp.net 5

@erichexter
Copy link
Copy Markdown

Bump. Is this thread still alive ?

@joe-udwin-lrn
Copy link
Copy Markdown

Bump. I would be interested seeing this fix being accepted.

@MrDark
Copy link
Copy Markdown

MrDark commented Feb 8, 2017

Can confirm this commit fixed our issue where the amount of connections went from a stable 20.000 to 200.000+ causing the server to be unavailable until restarted.

@moozzyk
Copy link
Copy Markdown
Contributor

moozzyk commented Feb 8, 2017

Looks like a better fix would be to make sure these events are wired up before SignalR starts reading data from the websocket (i.e. before this line)

@lucalooz
Copy link
Copy Markdown
Author

lucalooz commented Feb 10, 2017

Certainly there is a better method to fix this but at that time we needed a rapid fix for connection leaks instead of ditching SignalR

@moozzyk
Copy link
Copy Markdown
Contributor

moozzyk commented Feb 13, 2017

It was fixed by subscribing to events before starting reading data from websocket - cd37171

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.

7 participants