-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shut down with known-bad notification #4
Conversation
|
||
// TODO Time out? | ||
this.channel.closeFuture().await(); | ||
this.advanceToStateFromOriginStates(ClientState.EXIT, ClientState.SHUTDOWN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Looking back on it, this isn't quite right. Other things could cause the connection to close, and we'll want to make sure that we're actually closing because our known-bad notification was rejected. I will think on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, any rejected notification is good enough for our purposes. We just don't want to exit when the connection closes for a spurious IO problem.
…hods for legibility.
… from being rejected.
…ttempted to send a notification.
|
||
try { | ||
log.debug(String.format("%s waiting for connection to close.", this.getName())); | ||
this.disconnectOrContinueDisconnecting(); | ||
finishedDisconnecting = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be like the above connecting state finishedConnecting = this.connectOrContinueConnecting();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I now see that disconnectOrContinueDisconnecting()
does not return anything, so the assumption is just that if we attempted the disconnection it succeeded? Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fairer to say "if we didn't throw an InterruptedException
," but that's the general idea, yeah.
… APNs gateway does.
This is looking good to me. Is there anything else specific that I should take a look at on this pull request? I assume that the retrying approach not being without problems will be handled separately at a later date? If so I'm ready to give this a thumbs up. |
…ation Shut down with known-bad notification
The APNs protocol doesn't acknowledge successful notifications, so it's hard to know what's been accepted and what hasn't when a connection is shut down by the client. The APNs gateway does report errors, though, and closes the connection whenever something goes wrong. That behavior provides a circuitous, but more certain way of shutting connections.
The strategy introduced in this pull request is to write a known-bad notification to the APNs gateway and then wait for the remote host to close the connection. In the meantime, previously-written notifications may fail and get re-enqueued. When the known-bad notification is finally rejected, we know that everything sent before that point has been accounted for and everything else in the sent message buffer hasn't been processed yet.
TODO: