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

Check socket status before writing handshake response (may fix #74) #75

Closed
wants to merge 2 commits into from
Closed

Conversation

nicokaiser
Copy link
Contributor

I still don't get it, but my server ran into this if-statement twice this day, when it would otherwise crash because of #74.

@nicokaiser
Copy link
Contributor Author

Ok without this patch I get occasional "Error: EPIPE" crashes in the application (as the "write" happens before the application gets the chance to add an error handler).

When the socket is writable, either the write succeeds, or it fails, but at a time when there is either an error handler (the applications can set one immediately after the handshake) or the connection is already destroyed (invalid handshake, etc.).

@nicokaiser
Copy link
Contributor Author

Ok, but I'm still getting "Error: EPIPE" crashes.

@nicokaiser
Copy link
Contributor Author

@einaros You seem to remove all(!) listeners from the socket after closing (in cleanupWebsocketResources), even the default error handler that just destroys the socket. This may cause my occasional EPIPE errors...

Perhaps (!) this fixes that:
https://github.com/nicokaiser/ws/commit/f5c31d10a69c7075776c3165446adc8cf110472c

I'll try for a few hours and see if it crashes again...

@einaros
Copy link
Contributor

einaros commented Jun 5, 2012

Ah, yeah. I'll complete my fixes on this tonight, and include that patch as well. Sorry for the long delay!

@einaros
Copy link
Contributor

einaros commented Jun 12, 2012

I manually included updates which resemble the ones you've included here. Could your setup to master and see if stability improves?

@einaros
Copy link
Contributor

einaros commented Jun 14, 2012

Closing this - let's reiterate if the current fixes aren't sufficient :)

@einaros einaros closed this Jun 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants