search

redis / hiredis

Minimalistic C client for Redis >= 1.2
BSD 3-Clause "New" or "Revised" License
6.21k stars 1.81k forks source link

redis-connect: "poll(2): Interrupted system call (code 1)" #304

Closed WarrenWilkinson closed 9 years ago

WarrenWilkinson commented 9 years ago

I've been getting these errors from time to time when I make Redis connections. They correspond to EINTR.

It looks like this issue used to affect other hiredis commands, but was fixed: https://github.com/redis/hiredis/issues/99 https://github.com/redis/hiredis/pull/119

I looked at the RedisContextConnect function and I didn't see any any protection on that function... I think it might have been missed because it's called less often or maybe there is some other reason.

I'm using a version of hiredis downloaded from github a few days ago. My application creates connections at a rate of about 10 a second or so (closing them after about 2 seconds), and I operate like that continously for days --- so I'm more likely to run into this situation than other applications and I think it's a probablistic error.

If others agree this is an issue worth fixing, I can try to make/submit a pull request. If not, I'll just deal with the retry in my own code base.

mattsta commented 9 years ago

Sure, if it's a problem feel free to provide the fix here.

WarrenWilkinson commented 9 years ago

TLDR; Patch made, works, but do not accept it.

I've created a fix: https://github.com/WarrenWilkinson/hiredis/commit/bf02d397d785c4ee99acccd7c46a3d402989641b

(my patch basically duplicates what the GNU C libraries TEMP_FAILURE_RETRY command does -- http://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html#Interrupted-Primitives).

With this patch, the connection failures no longer occur. However, I believe hiredis might be better off returning EINTR. According to this site http://250bpm.com/blog:12, if you just capture EINTR and retry it'll break CTRL-C. The problem is that there is no way to tell Hiredis you're shutting down (and that it should stop the automatic retry) which you might want to do after you've caught CTRL-C.

I assume the patch for issue #99 is not subject to this problem as it appears the read is postponed, not restarted. But my assumption might be worth confirming if there are CTRL-C issues being reported.

For now, I'm going to close this issue because I believe Hiredis is a better citizen if it returns EINTR errors.