Send multiple notifications concurrently #54
Conversation
|
If we want this to be the default behaviour, what would that involve? A Service could have a worker pool. |
|
ATM I'm thinking:
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // notification to send. |
There was a problem hiding this comment.
move this internal stuff down
and update docs
|
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. |
| var workers uint | ||
| var number int | ||
|
|
||
| flag.StringVar(&deviceToken, "d", "", "Device token") |
There was a problem hiding this comment.
Nice good idea to add CLI control
|
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 |
| // wait group to wait for all responses | ||
| var wg sync.WaitGroup | ||
|
|
||
| // process responses |
There was a problem hiding this comment.
It's important that this is setup prior to sending notifications, otherwise there won't be room in the responses channel. Document this more thoroughly.
|
Other then a nit pick LGTM 👍 |
|
I experimented with NewNotification but I'm not in love with it. Also still considering supporting a synchronous push. |
| } | ||
|
|
||
| // NewClient sets up an HTTP/2 client for a certificate. | ||
| func newClient(cert tls.Certificate) (*http.Client, error) { | ||
| func NewClient(cert tls.Certificate) (*http.Client, error) { |
There was a problem hiding this comment.
Exposing NewClient again, as it was in v0.4.x
and improve handling of command line flags
| log.Fatal(err) | ||
| // process responses | ||
| go func() { | ||
| for { |
There was a problem hiding this comment.
There should probably be a signal to shutdown this goroutine. Hmm. #56
(not sure how useful this test is)
closes #31
closes #40