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

Listener fix #228

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Listener fix #228

wants to merge 4 commits into from

Conversation

notgiven688
Copy link
Contributor

In short: Fixes a problem with unnecessary listener socket restarts. Possible improvement of stability and memory, since Socket.Accept is not called in a recursive manner.

A solution for the problems mentioned here: #225

Should replace the pull-request made here: #226

@AppVeyorBot
Copy link

1 similar comment
@AppVeyorBot
Copy link

acceptDone.Set ();
OnClientConnect (s); },
e => { FleckLog.Error ("Error while listening for new clients", e);
if (RestartAfterListenError) TryRestart ();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this should properly separate listener and client errors, I'm not sure if having the restart is necessary.

@notgiven688
Copy link
Contributor Author

I am also not 100% sure. The idea is that a real servet socket error should be handled by a restart. In my tests this error never happened - so it is redundant in the worst case. We could remove it(?).

@notgiven688
Copy link
Contributor Author

I verified that the RestartOnError is still useful. In some cases people forgot to set appropriate file descriptor levels (they have ~50k clients). Here the websocket is correctly restarted after failing.

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