search

timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

Move notification struct error checking before queueing it in run loop #17

Closed bdotdub closed 9 years ago

bdotdub commented 9 years ago

This PR passes back validation and serialization errors synchronously to the caller of Send() instead of...well...ignoring it in the run loop or having to read off of another channel.

Fixes #4

bdotdub commented 9 years ago

@tylrtrmbl @nathany @brunodecarvalho if anyone has some spare :eyes:, this should help a bit with #4

nathany commented 9 years ago

@bdotdub If it's okay, I might not get to this until next Wednesday.

taylortrimble commented 9 years ago

Actually, I changed my mind back again from #4. I think it makes sense to use the synchronization and information passing mechanism you have now (a channel of Notification structs) and simply use FailedNotifs to report both user (ToBinary) and Apple (conn.Send) errors.

From my perspective, there's no reason I would need to know if the error came from conn.Send or somewhere else.

taylortrimble commented 9 years ago

What's up with Travis CI by the way?

nathany commented 9 years ago

Personally I'd prefer Send return errors when it can.

But then I'm trying to build a synchronous API around Send and FailedNotifs that gives me back Apple's response or a timeout.

bdotdub commented 9 years ago

I've been thinking a lot about this and am feeling like we should have Send() return all of the errors and forego FailedNotifs. If the client is disconnected from Apple, sending a notification shouldn't disappear into the run loop and eventually pop out of the other end through FailedNotifs.

The API feels cleaner to me if I get an error when trying to call Send() while we're disconnected. This will make it clear when we have connection issues with Apple and the caller code can check whether the error is ErrGatewayDisconnected or something and which would make the caller is responsible for calling Connect() in that case.

taylortrimble commented 9 years ago

You're suggesting a synchronous API then?

bdotdub commented 9 years ago

Yeah - it'll require a lot less bookkeeping internally and will push that option to the caller.

bdotdub commented 9 years ago

Especially since the actually sending is basically synchronous on one tcp connection

biasedbit commented 9 years ago

:+1: for the synchronous API/pushing that responsibility to the caller.

taylortrimble commented 9 years ago

:+1: For me too, actually. I actually prefer that the caller have granular control over stuff like that. Consider my previous comments as "assuming you wanted to keep it async".

Excited to see it now!

bdotdub commented 9 years ago

Going to hold off on merging this for now. I've got some code in the hopper that'll fixes up the internals to be less confusing as mentioned here.

nathany commented 9 years ago

This sounds pretty great and will really help make the API solid. But now I'm waiting for this to magically solve #16 instead of coming up with a solution.

nathany commented 9 years ago

Hoping #14 and #9 can get merged before going too far down this path.

nathany commented 9 years ago

@bdotdub Pushing some error handling back to user of the library sounds good. Say, if a certificate has expired, it shouldn't continue retrying (I've yet to confirm what the current behaviour is).

bdotdub commented 9 years ago

Closing this in favor of #31 and #33