RobotsAndPencils / buford

A push notification delivery engine for the new HTTP/2 APNS service.
MIT License
475 stars 52 forks source link

Send multiple notifications concurrently #54

Closed nathany closed 8 years ago

nathany commented 8 years ago

closes #31 closes #40

nathany commented 8 years ago

If we want this to be the default behaviour, what would that involve?

A Service could have a worker pool. The main thing is that responses are concurrent, so we need a channel in the API? Do we keep the existing synchronous APIs? Do both Push and PushBytes adopt this?

nathany commented 8 years ago

ATM I'm thinking:

nathany commented 8 years ago

Should push.notification be exported?

Should pushSync be exported?

Should there be an internal WaitGroup that Shutdown() waits on to ensure all responses have been received?

Using x/net/http2 directly instead of ConfigureTransport could allow support for Go 1.5.x (if desired), and make it so NewClient() doesn't return an error. https://github.com/sideshow/apns2/blob/master/client.go#L60

nathany commented 8 years ago

TODO: write tests that exercise the concurrency (multiple notifications) for testing with the race detector.

Look at where pointers are used or not used and decide which is preferable.

Reconsider how Error responses from Apple are provided. Could make the type conversion to *push.Error unnecessary.

curtisallen commented 8 years ago

Other then a nit pick LGTM 👍

nathany commented 8 years ago

I experimented with NewNotification but I'm not in love with it.

Also still considering supporting a synchronous push.