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

Prevent crashing due to SIGPIPE #9

Merged
merged 1 commit into from Dec 16, 2015

Conversation

briancroom
Copy link
Contributor

By default, calling send on a socket which has been closed on the remote end causes SIGPIPE to be sent to the process, which terminates it. These changes change the behavior to disable the signal, and instead make send return an error code if the socket has been closed.

This situation arises frequently when deploying to Cloud Foundry, as its healthcheck process opens a connection to the app and then closes without sending any data.

@kylef
Copy link
Member

kylef commented Dec 12, 2015

Thanks for the pull request, unfortunately this will only apply to Linux and not OS X since SO_NOSIGPIPE is not portable.

I'm wondering if a better approach could be just to override the signal handler for SIGPIPE and "do nothing", then send() should error from what I understand. This would at least work on all platforms.

What do you think @briancroom?

@briancroom
Copy link
Contributor Author

Hey @kylef, the first chunk in this patch, enabling SO_NOSIGPIPE handles OS X, while the second chunk with MSG_NOSIGNAL handles Linux. (I've been looking at e.g. here and here)

Now that you mention it, it does actually seem that it would be simpler to change the disposition of SIGPIPE to just ignore it (signal(SIGPIPE, SIG_IGN)), although I hesitate because it may not be appropriate for Curassow to mess with state that affects the entire process.

I guess I'd lean towards sticking with the current implementation, but I'd be happy to switch it over if you'd prefer!

kylef added a commit that referenced this pull request Dec 16, 2015
Prevent crashing due to SIGPIPE
@kylef kylef merged commit faebd06 into kylef-archive:master Dec 16, 2015
@kylef
Copy link
Member

kylef commented Dec 16, 2015

Okay that's great, I misread it initially and though they we're both #if os(Linux).

Thanks @briancroom!

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.

None yet

2 participants