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

allow to enable TCP keepalive #265

Closed
wants to merge 1 commit into from
Closed

allow to enable TCP keepalive #265

wants to merge 1 commit into from

Conversation

pedobry
Copy link

@pedobry pedobry commented Apr 6, 2016

Allow to enable TCP keepalive on socket, so OS can keep the persistent connection alive and avoid idle connection on firewalls.

@jchambers
Copy link
Owner

I think this is a problem worth solving, but I have two quibbles:

  1. Why not just turn on SO_KEEPALIVE all the time?
  2. You pointed out one TCP-level connection closure problem in interrupted persistent connection #264, but @abraxascorner pointed out a related-but-slightly-different problem in Connection reset by peer #257: connections will get closed by the APNs server if they sit idle for too long. I don't think SO_KEEPALIVE will save us there.

What I'd actually like to do is send HTTP/2 PING frames if the connection is idle for a while (a minute?); I think that should solve both problems, no? In addition, it will help us make sure the connection stays healthy at an application level, and it seems like that might be the better approach. I don't think there's any material harm in using both approaches, but if we take the PING approach, SO_KEEPALIVE would basically be vestigial, right?

@jmason
Copy link

jmason commented Apr 8, 2016

I would be in favour of HTTP-level PINGs rather than relying on TCP Keep-Alive -- the latter is only manipulable on a system-wide level in Linux, afaik, and defaults to 2 hours, which is probably far longer than most problematic firewalls' timeouts.

@pedobry
Copy link
Author

pedobry commented Apr 8, 2016

Technically, there is no harm in enabling SO_KEEPALIVE permanently. I just like the possibility to disable/enable it if I need to.
I agree that default 2 hours TCP keepalive timeout on Linux is unfortunate and enabling SO_KEEPALIVE alone won't help to keep connection open through most of the firewalls. Adjusting /proc/sys/net/ipv4/tcp_keepalive_time is necessary too.

Solution with PING would be far more elegant and simpler.
Issue #257 might or might not be related to Apple servers sending GOAWAY. As they might send unsolicited GOAWAY anytime for any of those reasons listed in API, it should be somehow handled too.

@jchambers
Copy link
Owner

As they might send unsolicited GOAWAY anytime for any of those reasons listed in API, it should be somehow handled too.

I'm preeeeeeeetty sure we do that even if we don't report it externally, but now I'm starting to second-guess things. I'll double-check on that.

@jchambers
Copy link
Owner

I'll double-check on that.

I double-checked, and this is all working as expected.

I went ahead and did the PING thing in #266. @pedobry with sincere thanks for the suggestion, I'm going to close this for now, since I think sending pings will be a more effective solution for more users. Sound good?

@pedobry
Copy link
Author

pedobry commented Apr 11, 2016

Yes. Using HTTP/2 PING is definitely better solution. Thanks for implementing this so quickly.

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

3 participants