Added methods to poll the state of the PushManager and its client threads

[Raibaz]

Referring to issue #2, I added a couple of methods to make the PushManager state explicit; they actually shouldn't be necessary, since the shutdown and start method already don't shut down the connection or open them if they're already in the expected state, but I feel like this way is a little clearer and the PushManager.isRunning() method may come in handy.

What do you think?

[jchambers]

I think this is a good idea, but would you mind adding a couple unit tests? I'd love to make sure that ApnsClientThread#isRunning and PushManager#isRunning return the right things (especially right at startup/shutdown). I don't doubt that everything is great right now; this is just for protection against regressions in the future.

Also (and I acknowledge this is nit-picky), would you mind adding a short body to the javadoc comments you added for stylistic consistency?

Thanks kindly!

[Raibaz]

I added javadoc bodies and implemented some unit tests, but I'm not 101% sure they're correct...let me know what you think :)

[jchambers]

I might add assertFalse(pm.isRunning()) before starting the push manager, but otherwise, I think this looks great!

[jchambers]

Oops -- mis-click. Sorry!

[Raibaz]

I guess your idea of maintaining an internal flag is probably better than mine, since the actual implementation has an issue: you're right in saying that ApnsClientThread.isRunning should not be true if the thread state is CONNECT, but when the PushManager isn't started yet its threads are already in state CONNECT even if the PushManager hasn't been started and thus, in my mind, is not running.

We could solve this problem adding a NEW ClientState representing the state when the PushManager has been created but not started yet, but I guess adding a single boolean flag in the PushManager indicating if it has been started or not and if it is shutting down would probably be easier.

Going to do this as soon as I find some time.

[jchambers]

Going to do this as soon as I find some time.

Cool. Thanks for sticking with it!

We could solve this problem adding a NEW ClientState representing the state when the PushManager has been created but not started yet…

Actually, I think I'm going to need to do that in #4 anyhow. Will keep you posted.

[jchambers]

So I've made the check-client-state strategy even less feasible in #4 because I, uh, maybe got rid of ApnsClientThread.clientState. Instead, client state is stored in a local variable in the run method. The change should make the internal state machine much more readable and also means that when we DO shut down, we'll have a much better idea of the state of all notifications that had been sent up to that point. That said, I think it does mean that the best (only?) option for tracking PushManager state is by a flag maintained by the PushManager itself.

[jchambers]

@Raibaz Hate to say it, but we merged #4 and now there are some conflicts here.

[Raibaz]

I don't have a real connection available at the moment, will have a look
and solve next week
Il 02/nov/2013 19:30 "Jon Chambers" notifications@github.com ha scritto:

@Raibaz https://github.com/Raibaz Hate to say it, but we merged #4https://github.com/relayrides/pushy/pull/4and now there are some conflicts here.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-27627969
.

[jchambers]

@Raibaz Closing this because I think we've got this covered in cc9b69f. Please let me know if you disagree.

[Raibaz]

Yep makes sense to me.