Close idle channels instead of sending PING frames

pushy/src/main/java/com/eatthepath/pushy/apns/ApnsChannelFactory.java

@@ -116,12 +116,10 @@ protected void initChannel(final SocketChannel channel) {
                         clientHandlerBuilder = new TokenAuthenticationApnsClientHandler.TokenAuthenticationApnsClientHandlerBuilder()
                                 .signingKey(clientConfiguration.getSigningKey().get())
                                 .tokenExpiration(clientConfiguration.getTokenExpiration())
-                                .authority(authority)
-                                .idlePingInterval(clientConfiguration.getIdlePingInterval());
+                                .authority(authority);
                     } else {
                         clientHandlerBuilder = new ApnsClientHandler.ApnsClientHandlerBuilder()
-                                .authority(authority)
-                                .idlePingInterval(clientConfiguration.getIdlePingInterval());
+                                .authority(authority);
                     }
 
                     clientConfiguration.getFrameLogger().ifPresent(clientHandlerBuilder::frameLogger);
@@ -139,7 +137,7 @@ protected void initChannel(final SocketChannel channel) {
 
                 pipeline.addLast(sslHandler);
                 pipeline.addLast(new FlushConsolidationHandler(FlushConsolidationHandler.DEFAULT_EXPLICIT_FLUSH_AFTER_FLUSHES, true));
-                pipeline.addLast(new IdleStateHandler(clientConfiguration.getIdlePingInterval().toMillis(), 0, 0, TimeUnit.MILLISECONDS));
+                pipeline.addLast(new IdleStateHandler(clientConfiguration.getCloseAfterIdleDuration().toMillis(), 0, 0, TimeUnit.MILLISECONDS));
                 pipeline.addLast(apnsClientHandler);
             }
         });

pushy/src/main/java/com/eatthepath/pushy/apns/ApnsClientBuilder.java

@@ -79,19 +79,19 @@ public class ApnsClientBuilder {
     private ProxyHandlerFactory proxyHandlerFactory;
 
     private Duration connectionTimeout;
-    private Duration idlePingInterval = DEFAULT_IDLE_PING_INTERVAL;
+    private Duration closeAfterIdleDuration = DEFAULT_CLOSE_AFTER_IDLE_DURATION;
     private Duration gracefulShutdownTimeout;
 
     private Http2FrameLogger frameLogger;
 
     private boolean useAlpn = false;
 
     /**
-     * The default idle time in milliseconds after which the client will send a PING frame to the APNs server.
+     * The default idle time after which the client will close a connection (which may be reopened later).
      *
      * @since 0.11
      */
-    public static final Duration DEFAULT_IDLE_PING_INTERVAL = Duration.ofMinutes(1);
+    public static final Duration DEFAULT_CLOSE_AFTER_IDLE_DURATION = Duration.ofMinutes(1);
 
     /**
      * The hostname for the production APNs gateway.
@@ -476,18 +476,18 @@ public ApnsClientBuilder setConnectionTimeout(final Duration timeout) {
     }
 
     /**
-     * Sets the amount of idle time (in milliseconds) after which the client under construction will send a PING frame
-     * to the APNs server. By default, clients will send a PING frame after an idle period of
-     * {@link com.eatthepath.pushy.apns.ApnsClientBuilder#DEFAULT_IDLE_PING_INTERVAL}.
+     * Sets the amount of idle time after which the client under construction will close a connection (which may be
+     * reopened later). By default, clients will close connections after an idle time of
+     * {@link com.eatthepath.pushy.apns.ApnsClientBuilder#DEFAULT_CLOSE_AFTER_IDLE_DURATION}.
      *
-     * @param idlePingInterval the amount of idle time after which the client will send a PING frame
+     * @param closeAfterIdleDuration the amount of idle time after which the client will close a connection
      *
      * @return a reference to this builder
      *
-     * @since 0.10
+     * @since 0.16.0
      */
-    public ApnsClientBuilder setIdlePingInterval(final Duration idlePingInterval) {
-        this.idlePingInterval = idlePingInterval;
+    public ApnsClientBuilder setCloseAfterIdleDuration(final Duration closeAfterIdleDuration) {
+        this.closeAfterIdleDuration = closeAfterIdleDuration;
         return this;
     }
 
@@ -628,7 +628,7 @@ public ApnsClient build() throws SSLException {
                             this.tokenExpiration,
                             this.proxyHandlerFactory,
                             this.connectionTimeout,
-                            this.idlePingInterval,
+                            this.closeAfterIdleDuration,
                             this.gracefulShutdownTimeout,
                             this.concurrentConnections,
                             this.metricsListener,

pushy/src/main/java/com/eatthepath/pushy/apns/ApnsClientConfiguration.java

@@ -46,7 +46,7 @@ class ApnsClientConfiguration {
     private final Duration tokenExpiration;
     private final ProxyHandlerFactory proxyHandlerFactory;
     private final Duration connectionTimeout;
-    private final Duration idlePingInterval;
+    private final Duration closeAfterIdleDuration;
     private final Duration gracefulShutdownTimeout;
     private final int concurrentConnections;
     private final ApnsClientMetricsListener metricsListener;
@@ -59,7 +59,7 @@ public ApnsClientConfiguration(final InetSocketAddress apnsServerAddress,
                                    final Duration tokenExpiration,
                                    final ProxyHandlerFactory proxyHandlerFactory,
                                    final Duration connectionTimeout,
-                                   final Duration idlePingInterval,
+                                   final Duration closeAfterIdleDuration,
                                    final Duration gracefulShutdownTimeout,
                                    final int concurrentConnections,
                                    final ApnsClientMetricsListener metricsListener,
@@ -72,7 +72,7 @@ public ApnsClientConfiguration(final InetSocketAddress apnsServerAddress,
         this.tokenExpiration = tokenExpiration != null ? tokenExpiration : DEFAULT_TOKEN_EXPIRATION;
         this.proxyHandlerFactory = proxyHandlerFactory;
         this.connectionTimeout = connectionTimeout;
-        this.idlePingInterval = idlePingInterval;
+        this.closeAfterIdleDuration = closeAfterIdleDuration;
         this.gracefulShutdownTimeout = gracefulShutdownTimeout;
         this.concurrentConnections = concurrentConnections;
         this.metricsListener = metricsListener;
@@ -107,8 +107,8 @@ public Optional<Duration> getConnectionTimeout() {
         return Optional.ofNullable(connectionTimeout);
     }
 
-    public Duration getIdlePingInterval() {
-        return idlePingInterval;
+    public Duration getCloseAfterIdleDuration() {
+        return closeAfterIdleDuration;
     }
 
     public Optional<Duration> getGracefulShutdownTimeout() {

pushy/src/main/java/com/eatthepath/pushy/apns/ApnsClientHandler.java

@@ -38,22 +38,18 @@
 import io.netty.handler.timeout.IdleStateEvent;
 import io.netty.util.AsciiString;
 import io.netty.util.collection.IntObjectHashMap;
-import io.netty.util.concurrent.GenericFutureListener;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.concurrent.PromiseCombiner;
-import io.netty.util.concurrent.ScheduledFuture;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.text.ParseException;
-import java.time.Duration;
 import java.util.Map;
 import java.util.Objects;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.TimeUnit;
 
 class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameListener, Http2Connection.Listener {
 
@@ -65,9 +61,6 @@ class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameList
 
     private final String authority;
 
-    private final Duration pingTimeout;
-    private ScheduledFuture<?> pingTimeoutFuture;
-
     private Throwable connectionErrorCause;
 
     private static final AsciiString APNS_PATH_PREFIX = new AsciiString("/3/device/");
@@ -92,7 +85,6 @@ class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameList
     public static class ApnsClientHandlerBuilder extends AbstractHttp2ConnectionHandlerBuilder<ApnsClientHandler, ApnsClientHandlerBuilder> {
 
         private String authority;
-        private Duration idlePingInterval;
 
         ApnsClientHandlerBuilder authority(final String authority) {
             this.authority = authority;
@@ -103,15 +95,6 @@ String authority() {
             return this.authority;
         }
 
-        Duration idlePingInterval() {
-            return idlePingInterval;
-        }
-
-        ApnsClientHandlerBuilder idlePingInterval(final Duration idlePingIntervalMillis) {
-            this.idlePingInterval = idlePingIntervalMillis;
-            return this;
-        }
-
         @Override
         public ApnsClientHandlerBuilder frameLogger(final Http2FrameLogger frameLogger) {
             return super.frameLogger(frameLogger);
@@ -136,7 +119,7 @@ protected boolean encoderEnforceMaxConcurrentStreams() {
         public ApnsClientHandler build(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings) {
             Objects.requireNonNull(this.authority(), "Authority must be set before building an ApnsClientHandler.");
 
-            final ApnsClientHandler handler = new ApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.idlePingInterval());
+            final ApnsClientHandler handler = new ApnsClientHandler(decoder, encoder, initialSettings, this.authority());
             this.frameListener(handler);
             return handler;
         }
@@ -147,7 +130,7 @@ public ApnsClientHandler build() {
         }
     }
 
-    ApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final Duration idlePingInterval) {
+    ApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority) {
         super(decoder, encoder, initialSettings);
 
         this.authority = authority;
@@ -157,8 +140,6 @@ public ApnsClientHandler build() {
         this.streamErrorCausePropertyKey = this.connection().newKey();
 
         this.connection().addListener(this);
-
-        this.pingTimeout = idlePingInterval.dividedBy(2);
     }
 
     @Override
@@ -264,22 +245,8 @@ protected Http2Headers getHeadersForPushNotification(final ApnsPushNotification
     @Override
     public void userEventTriggered(final ChannelHandlerContext context, final Object event) throws Exception {
         if (event instanceof IdleStateEvent) {
-            log.trace("Sending ping due to inactivity.");
-
-            this.encoder().writePing(context, false, System.currentTimeMillis(), context.newPromise()).addListener(
-                    (GenericFutureListener<ChannelFuture>) future -> {
-                        if (!future.isSuccess()) {
-                            log.debug("Failed to write PING frame.", future.cause());
-                            future.channel().close();
-                        }
-                    });
-
-            this.pingTimeoutFuture = context.channel().eventLoop().schedule(() -> {
-                log.debug("Closing channel due to ping timeout.");
-                context.channel().close();
-            }, pingTimeout.toMillis(), TimeUnit.MILLISECONDS);
-
-            this.flush(context);
+            log.debug("Closing idle channel.");
+            context.close();
         }
 
         super.userEventTriggered(context, event);
@@ -413,11 +380,6 @@ public void onPingRead(final ChannelHandlerContext ctx, final long pingData) {
 
     @Override
     public void onPingAckRead(final ChannelHandlerContext context, final long pingData) {
-        if (this.pingTimeoutFuture != null) {
-            this.pingTimeoutFuture.cancel(false);
-        } else {
-            log.error("Received PING ACK, but no corresponding outbound PING found.");
-        }
     }
 
     @Override
@@ -508,10 +470,6 @@ protected void onConnectionError(final ChannelHandlerContext context, final bool
 
     @Override
     public void channelInactive(final ChannelHandlerContext context) throws Exception {
-        if (this.pingTimeoutFuture != null) {
-            this.pingTimeoutFuture.cancel(false);
-        }
-
         for (final PushNotificationFuture<?, ?> future : this.unattachedResponsePromisesByStreamId.values()) {
             future.completeExceptionally(STREAM_CLOSED_BEFORE_REPLY_EXCEPTION);
         }

pushy/src/main/java/com/eatthepath/pushy/apns/TokenAuthenticationApnsClientHandler.java

@@ -81,14 +81,14 @@ public ApnsClientHandler build(final Http2ConnectionDecoder decoder, final Http2
             Objects.requireNonNull(this.signingKey(), "Signing key must be set before building a TokenAuthenticationApnsClientHandler.");
             Objects.requireNonNull(this.tokenExpiration(), "Token expiration duration must be set before building a TokenAuthenticationApnsClientHandler.");
 
-            final ApnsClientHandler handler = new TokenAuthenticationApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.idlePingInterval(), this.signingKey(), this.tokenExpiration());
+            final ApnsClientHandler handler = new TokenAuthenticationApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.signingKey(), this.tokenExpiration());
             this.frameListener(handler);
             return handler;
         }
     }
 
-    protected TokenAuthenticationApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final Duration idlePingInterval, final ApnsSigningKey signingKey, final Duration tokenExpiration) {
-        super(decoder, encoder, initialSettings, authority, idlePingInterval);
+    protected TokenAuthenticationApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final ApnsSigningKey signingKey, final Duration tokenExpiration) {
+        super(decoder, encoder, initialSettings, authority);
 
         this.signingKey = Objects.requireNonNull(signingKey, "Signing key must not be null for token-based client handlers.");
         this.tokenExpiration = Objects.requireNonNull(tokenExpiration, "Token expiration must not be null for token-based client handlers");