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

SIGPIPE on background thread can cause segfault #168

Closed
wch opened this issue Aug 14, 2018 · 5 comments · Fixed by #169
Closed

SIGPIPE on background thread can cause segfault #168

wch opened this issue Aug 14, 2018 · 5 comments · Fixed by #169

Comments

@wch
Copy link
Collaborator

wch commented Aug 14, 2018

Originally from rstudio/plumber#289. A SIGPIPE on the httpuv background thread can cause R to segfault.

@jcheng5
Copy link
Member

jcheng5 commented Aug 14, 2018

What!? I could've sworn we sigignored it!

@jcheng5
Copy link
Member

jcheng5 commented Aug 14, 2018

Need to ignore in later too

@wch
Copy link
Collaborator Author

wch commented Aug 14, 2018

We lost the signal(SIGPIPE, SIG_IGN); in the transition to httpuv on a background thread. I have a potential fix that blocks SIGPIPE only on the httpuv background thread, so that it doesn't interfere with other SIGPIPE stuff in R.

wch added a commit that referenced this issue Aug 14, 2018
@jcheng5
Copy link
Member

jcheng5 commented Aug 14, 2018

We used to ignore on the main thread, right? We might want to consider just sticking with that (or whatever it is that we used to do), I remember spending a lot of time and energy deciding on the right course of action there.

@wch
Copy link
Collaborator Author

wch commented Aug 14, 2018

We did used to ignore at the process level, each time we were about to do a uv_run() (which was called from the main thread):
https://github.com/rstudio/httpuv/blob/v1.3.6/src/httpuv.cpp#L400-L404

However, R itself apparently does have a SIGPIPE handler and so I think it would be best not to mess with R's SIGPIPE handling if possible.

Somewhat related: s-u/Rserve#100 (comment)

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 a pull request may close this issue.

2 participants