-
Notifications
You must be signed in to change notification settings - Fork 448
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
Optionally close connections when inactive #116
Conversation
…Doesn't have teeth yet.
…scribed amount of time.
…tempt times out. Still need to add tests.
…es/pushy into close_after_send_attempt_limit Conflicts: src/main/java/com/relayrides/pushy/apns/ApnsConnection.java src/main/java/com/relayrides/pushy/apns/ApnsConnectionConfiguration.java
…ctions_when_inactive
…es/pushy into close_after_send_attempt_limit
…es/pushy into graceful_shutdown_timeout
…es/pushy into close_after_send_attempt_limit
@@ -92,6 +95,9 @@ | |||
private boolean rejectionReceived = false; | |||
private final SentNotificationBuffer<T> sentNotificationBuffer; | |||
|
|||
private static final String PIPELINE_MAIN_HANDLER = "handler"; | |||
private static final String PIPELINE_IDLE_STATE_HANLDER = "idleStateHandler"; |
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.
Minor: sp - HANLDER
-> HANDLER
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.
Oops -- good catch. Fixed in b3b6e31.
@@ -423,6 +444,13 @@ public void operationComplete(final Future<Channel> handshakeFuture) { | |||
if (apnsConnection.listener != null) { | |||
apnsConnection.listener.handleConnectionSuccess(apnsConnection); | |||
} | |||
|
|||
if (apnsConnection.configuration.getCloseAfterInactivityTime() != null) { |
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.
Can configuration
be null as well? If so, an extra null check for that as well would be great.
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.
Can configuration be null as well?
No, but I made that clearer in 2b4848d.
There's already a test to make sure constructing a connection fails with a NullPointerException
, but that was happening by way of making an unchecked read to the given configuration object (we would try to blindly read the sent notification buffer size) rather than checking for null configurations directly.
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.
Oops -- f5ab059, too.
* configuration will be closed. If {@code null}, connections created with this configuration will never be closed | ||
* due to inactivity. | ||
* | ||
* @return the time, in seconds, since the last push notification was sent after which connections created with 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.
Minor: A little rephrasing might make this clearer. What about @return the time, in seconds, between the last push notification and connection closure
?
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.
Yeah -- that's way better. Done in 10ede21.
👍 |
…es/pushy into graceful_shutdown_timeout Conflicts: src/main/java/com/relayrides/pushy/apns/ApnsConnection.java src/main/java/com/relayrides/pushy/apns/ApnsConnectionConfiguration.java
Graceful shutdown timeouts
…es/pushy into close_after_send_attempt_limit Conflicts: src/main/java/com/relayrides/pushy/apns/ApnsConnectionConfiguration.java src/test/java/com/relayrides/pushy/apns/ApnsConnectionTest.java
…ionConfiguration.
Optionally close connections after a set number of send attempts
…ctions_when_inactive Conflicts: src/main/java/com/relayrides/pushy/apns/ApnsConnection.java src/test/java/com/relayrides/pushy/apns/ApnsConnectionTest.java
Optionally close connections when inactive
This is the first part in a series of changes to work around the inability to determine the state of an APNs connection. Because we can't determine if a channel is "alive" or not except by sending a known-bad notification (thereby closing the connection), the big idea is to close connections more frequently. In this pull request, we allow connections to close after a user-configurable period of inactivity.
One consideration here is that, if a push manager is completely idle, it might just sit there opening and closing connections at regular intervals. That seems pretty okay, to me, though.
EDIT: Also, please pardon all of the auto-formatting stuff (like
@Override
annotations) that's happening since we moved from Java 1.5 to Java 1.6.