Open bdotdub opened 9 years ago
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.
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?
@tylrtrmbl sure! I'll open up a PR with a large DO NOT MERGE disclaimer :)
It's up here now #33
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?
Please consider enabling perf customization as discussed in #32.
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.
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.
@tylrtrmbl You said you had some comments on #33?
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.
That would be great.
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:
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?
API changes:
Here's an idea:
Right now we have, roughly:
These are composed as client.session.connection
.
I suggest something a little different:
Send(n Notification)
to Write(b []byte)
Transport
and build up all their own error handling logicIdeally, this will help support both naïve and advanced users of the library.
I would like to separate out some different pieces:
Beyond that, I think it still makes sense to have Transport (Session) and a higher-level Client that is long-lived.
It makes sense to have one buffer per connection/session like how @bdotdub has done it.
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, theClient
maintains the whole lifecycle of the interactions with Apple (connection, reconnection, requeueing, etc). WhileClient
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
is now an interface so that it can be mockable.Session
.Client
is now only responsible for holding aConn
and then createSessions
to interact with.Client
holds the higher level logic of creating a newSession
on disconnect and requeueing undelivered notifs. By extracting out theSession
bits, it'll be much easier to be able to provide ways to change reconnection policies, strategies for how to deal with undelivered notifs, etcIn theory, you could easily create your own
Client
with your own behavior by usingSession
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