Close connections after receiving an InternalServerError

[jchambers]

For a long time, we've been working around #408, an upstream issue where connections could go into a permanently-unhealthy state by responding to all notifications with an InternalServerError after replacing an expired authentication token. We hesitated to build a fix into Pushy itself for several reasons:

1. We had reason to believe that the issue would receive a timely upstream fix, and so it was best not to commit too hard to a client-side fix.
2. It wasn't obvious that the same solution would work for everybody.
3. Users had a viable workaround in that they could close and re-open connections if things got stuck in an unhealthy state.

Now that we've moved to a connection-pooling model, users no longer have direct control over connections. We've also gone a looooooong time without an upstream fix, and the new connection model allows for easy behind-the-scenes connection replacement. This means that we have a more acute need for a client-side fix and fewer reasons to hold off.

Long story short: this pull request introduces behavior where clients will quietly replace unhealthy connections behind the scenes, which fixes #408.

[ksquaredkey]

I was digging into this a bit earlier today and came up with an idea that might be easier to implement. The APNS Spec says we are supposed to create a new auth token after 1 hour of use of the previous token. So it seems to me that in TokenAuthenticationApnsClientHandler.java we should be storing both the "this.authenticationToken" but also this "this.authenticationTokenIssuedAt". Then in "getheadersForPushNotification()" we modify the test from:

java

if (this.authenticationToken == null) {

to something like:

if ((this.authenticationToken == null) ||
  (new Date().getTime() - this.authenticationTokenIssuedAt.getTime()) > (3600L * 1000L)) {

This is just off the top of my head. I'm not a java person so please pardon if my time calculation is ugly, but I think it shows the idea. I'd probably put the issuedAt inside the authenticationToken rather than as another value in the TokenAuthenticationApnsClientHandler, etc.

This does not really obviate the need for handling the server error case, but I think with the above and given the APNS spec we might reduce the number of times we have to handle it and have to do two round-trips for the push notification that happens to hit the expired token.

Thoughts?

[jchambers]

@ksquaredkey Thanks for the suggestion! If I'm understanding right, the core of your suggestion is that we should proactively generate new tokens at the one-hour mark instead of waiting for the server to tell us that tokens have expired. That was actually something we did early on (but never shipped), but we couldn't guarantee that the server wouldn't expire tokens at some other time (i.e. they might change the the expiration period to 30 minutes instead of an hour), and so we'd need to leave the regenerate-on-expiration code in place anyhow. Rather than having two pathways to achieve the same thing, we decided to keep it simple.

To be clear, Pushy handles this all internally and automatically re-sends notifications with expired tokens. Callers should never know that it even happened. It's true that means we'll double-send some notifications, but even if we're looking at a slow round-trip time of ~100 milliseconds, that means we're "vulnerable" for 0.1 seconds / 3600 seconds ~= 0.003% of the time, and that seems pretty okay to me.

Am I addressing what you're getting at, or am I missing your point?

[jchambers]

…either way, I think the need for this change is pressing, and revising our overall strategy for token expiration is more than we should take on as part of this effort. I'm going to go ahead and merge/ship this change.

@ksquaredkey I'd love to learn more about your suggestion, but let's plan to address it as a separate thing. Thanks!

[ksquaredkey]

Sounds good. I'll work up a proof-of-concept as a separate pull request.