Client and Session separation

[bdotdub]

Hey y'all!

Sorry for the radio silence on a bunch of the issues & PRs. As mentioned in #17, I've been thinking about how to make it a bit more synchronous to make the data flow so that the error reporting and handling isn't as roundabout. This feels like the root of the problem for #4, #19, and #25.

In the current implementation, it feels like I've conflated multiple concepts into a single Client struct. Right now, the Client maintains the whole lifecycle of the interactions with Apple (connection, reconnection, requeueing, etc). While Client has an arguably convenient interface, it doesn't allow for granular control for what to do for connection retry strategies, reconnection behavior (ie. do we automatically requeue all failed notifs?), etc.

On the branch I've been working on (Branch — Comparison to master), I've added a separate Session concept. The state of the branch is that most of the code is there, and I'm now just filling in the rest of the tests.

At a high level, here is what changed:

• Conn – connects to Apple's gateways and holds the TLS config. The main change here is that Conn is now an interface so that it can be mockable.
• Session – this represents a single stateful lifecycle of Connect > Send notifs > Disconnect with the APNS servers. Once it disconnects (explicit via the caller, or implicit via APN errors), it can no longer be revived and reused. What this allows us to do is to allow a way to pull out the requeueble notifications and decide what to do with it – that is in contrast to what happens now (automatically requeues them on reconnection). This, again, is also an interface for mockability.
• Client – largely the same interface from before. It is meant to still be really convenient for the caller. The difference in implementation is that the grisly bits of a single session to APNS is now in Session. Client is now only responsible for holding a Conn and then create Sessions to interact with. Client holds the higher level logic of creating a new Session on disconnect and requeueing undelivered notifs. By extracting out the Session bits, it'll be much easier to be able to provide ways to change reconnection policies, strategies for how to deal with undelivered notifs, etc

In theory, you could easily create your own Client with your own behavior by using Session if you didn't like the one in this lib.

These changes have also helped the testability and brittleness of the previous tests. They should now be running consistently on Travis.

I'd love to get all of your thoughts on this!

cc @brunodecarvalho @tylrtrmbl @nathany @willfaught

In the meantime, I'll take a look at the PRs currently open

[nathany]

Wow Benny. This sounds great.

Let us know when we should try this branch out from our apps, and if you would like a code review.

[taylortrimble]

I have so many comments, but some of them are better as line comments. Do you feel like opening a pull request for that diff?

[bdotdub]

@tylrtrmbl sure! I'll open up a PR with a large DO NOT MERGE disclaimer :)

[bdotdub]

It's up here now #33

[willfaught]

Sounds promising!

The new interfaces are great. Making Client a mockable interface would greatly ease testing. When I test a component that uses this client, the only thing I really care about mocking is the Send call, since the client setup can happen beforehand elsewhere (at least the way I use it :). For example:

// setup mytestapnsclient to use a fake Conn/Session
...
mytestapnsclient.When("Send", apns.Notification{...}).Return(errors.New(...))
...
if err := myproduct.LaunchCampaign(mytestapnsclient); err == nil {
    t.Error("should return client error")
}

Regarding the Client/Session split: Can multiple Clients share one Session? Does a Client create a second Session after closing the first one to resend unsent notifications? Why does Client have the Conn if Session has the Connect/Disconnect methods?

[willfaught]

Please consider enabling perf customization as discussed in #32.

[nathany]

I've renamed #33 to a "develop" branch and rebased it on master. Feel free to submit pull requests to that branch. Let's try to address the comments and questions there and get this merged in.

@willfaught Let's focus on the client/session separation and come back to #32 after it is wrapped up and merged to master.

[nathany]

I'm not totally sure about removing FailedNotifs. It would be nice to provide a synchronous API with Send() returning errors, but I'm not sure Apple's response allows this to work that way?

Say, if there is an "8 Invalid token" error, at what point will the response come over the channel and how long do we wait? There are advantages to fire-and-forget as well.

Might be worth checking other implementations.

https://developer.apple.com/library/ios/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/Chapters/CommunicatingWIthAPS.html#//apple_ref/doc/uid/TP40008194-CH101-SW4

[nathany]

@tylrtrmbl You said you had some comments on #33?

[taylortrimble]

Yeah! I caught up with @bdotdub over Google Hangouts and we scratched out some ideas. I'd be happy to go over them with you, I think I still have my notes.

[nathany]

That would be great.

[nathany]

It would be really nice to wrap this up and merge it in, seeing as it solves the failed tests on Travis CI.

There are still a lot of other things I'd like to followup with. But for now I wonder if we:

• [ ] Add FailedNotifs back into Client (at least for now)
• [ ] Leave the retry policies out, basically keep the same behaviour as master.
• [ ] Don't export Session yet, keep it internal for now.

[nathany]

The state of the branch is that most of the code is there, and I'm now just filling in the rest of the tests.

@bdotdub Specifically, what additional tests would you like added?

[nathany]

API changes:

• Conn no longer exposes Close, Connect, Read, Write (or it's not showing up in the docs)
• ~~NotificationResult is now SessionError~~

[taylortrimble]

Here's an idea:

Right now we have, roughly:

• Connection: Nothing special
• Session: By default, thing that has reconnection and replay logic for dealing with errors
  • Can be subbed out for different reconnection and replay logic
  • The default one keeps a circular buffer of notifs for last-N replay
• Client: Thing that sends notifications (through a session)

These are composed as client.session.connection.

I suggest something a little different:

• Connection: Nothing special
• Transport: Sends notification (through a connection)
  • Basically just proxies Send(n Notification) to Write(b []byte)
  • Synchronous
  • Can stand alone to send Notifications
• Client: Give it a connection provider (see #49), and it will handle reconnection and replay logic
  • Keeps a circular buffer of notifs for last-N replay

Current Version Overview

• Default customer just uses Client
• Advanced customer creates a custom session, perhaps custom connection

Proposed Version Overview

• Default customer just uses Client
• Advanced customer makes use of the synchronous Transport and build up all their own error handling logic

Ideally, this will help support both naïve and advanced users of the library.

[nathany]

I would like to separate out some different pieces:

• The JSON/binary encoding of the notification. I'd like to prepare them in advance and store the binary glob to send Apple in our circular buffer [UPDATE: The problem with this is being able to report the failed notification back to the caller]. Also I'd like to be able to test the encoding step without sending (or mocking send).
• I would like to define some hooks for a before notification sent or an error occurring. Then make a separate component with the circular buffer and replay logic that plugs into these hooks. Ideally different replay logic could be plugged in (eg. store in Redis) or hooks could be used for other purposes (UPDATE: like logging https://github.com/timehop/apns/issues/47#issuecomment-102552264).

Beyond that, I think it still makes sense to have Transport (Session) and a higher-level Client that is long-lived.

[nathany]

It makes sense to have one buffer per connection/session like how @bdotdub has done it.

• When replaying (on disconnect), there is a fresh buffer storing messages sent while being replayed out of the old buffer.
• If there are multiple connections to Apple for sending messages, each one should have a separate buffer. One connection disconnecting shouldn't impact the others. (though this could be multiple Clients)