Skip to content

Commit 4a0d046

Browse files
jchambersjchambers
committed
Close idle channels instead of sending PING frames
1 parent 2c43ec3 commit 4a0d046

File tree

5 files changed

+26
-70
lines changed

5 files changed

+26
-70
lines changed

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -116,12 +116,10 @@ protected void initChannel(final SocketChannel channel) {
116116
clientHandlerBuilder = new TokenAuthenticationApnsClientHandler.TokenAuthenticationApnsClientHandlerBuilder()
117117
.signingKey(clientConfiguration.getSigningKey().get())
118118
.tokenExpiration(clientConfiguration.getTokenExpiration())
119-
.authority(authority)
120-
.idlePingInterval(clientConfiguration.getIdlePingInterval());
119+
.authority(authority);
121120
} else {
122121
clientHandlerBuilder = new ApnsClientHandler.ApnsClientHandlerBuilder()
123-
.authority(authority)
124-
.idlePingInterval(clientConfiguration.getIdlePingInterval());
122+
.authority(authority);
125123
}
126124

127125
clientConfiguration.getFrameLogger().ifPresent(clientHandlerBuilder::frameLogger);
@@ -139,7 +137,7 @@ protected void initChannel(final SocketChannel channel) {
139137

140138
pipeline.addLast(sslHandler);
141139
pipeline.addLast(new FlushConsolidationHandler(FlushConsolidationHandler.DEFAULT_EXPLICIT_FLUSH_AFTER_FLUSHES, true));
142-
pipeline.addLast(new IdleStateHandler(clientConfiguration.getIdlePingInterval().toMillis(), 0, 0, TimeUnit.MILLISECONDS));
140+
pipeline.addLast(new IdleStateHandler(clientConfiguration.getCloseAfterIdleDuration().toMillis(), 0, 0, TimeUnit.MILLISECONDS));
143141
pipeline.addLast(apnsClientHandler);
144142
}
145143
});

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

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -79,19 +79,19 @@ public class ApnsClientBuilder {
7979
private ProxyHandlerFactory proxyHandlerFactory;
8080

8181
private Duration connectionTimeout;
82-
private Duration idlePingInterval = DEFAULT_IDLE_PING_INTERVAL;
82+
private Duration closeAfterIdleDuration = DEFAULT_CLOSE_AFTER_IDLE_DURATION;
8383
private Duration gracefulShutdownTimeout;
8484

8585
private Http2FrameLogger frameLogger;
8686

8787
private boolean useAlpn = false;
8888

8989
/**
90-
* The default idle time in milliseconds after which the client will send a PING frame to the APNs server.
90+
* The default idle time after which the client will close a connection (which may be reopened later).
9191
*
9292
* @since 0.11
9393
*/
94-
public static final Duration DEFAULT_IDLE_PING_INTERVAL = Duration.ofMinutes(1);
94+
public static final Duration DEFAULT_CLOSE_AFTER_IDLE_DURATION = Duration.ofMinutes(1);
9595

9696
/**
9797
* The hostname for the production APNs gateway.
@@ -476,18 +476,18 @@ public ApnsClientBuilder setConnectionTimeout(final Duration timeout) {
476476
}
477477

478478
/**
479-
* Sets the amount of idle time (in milliseconds) after which the client under construction will send a PING frame
480-
* to the APNs server. By default, clients will send a PING frame after an idle period of
481-
* {@link com.eatthepath.pushy.apns.ApnsClientBuilder#DEFAULT_IDLE_PING_INTERVAL}.
479+
* Sets the amount of idle time after which the client under construction will close a connection (which may be
480+
* reopened later). By default, clients will close connections after an idle time of
481+
* {@link com.eatthepath.pushy.apns.ApnsClientBuilder#DEFAULT_CLOSE_AFTER_IDLE_DURATION}.
482482
*
483-
* @param idlePingInterval the amount of idle time after which the client will send a PING frame
483+
* @param closeAfterIdleDuration the amount of idle time after which the client will close a connection
484484
*
485485
* @return a reference to this builder
486486
*
487-
* @since 0.10
487+
* @since 0.16.0
488488
*/
489-
public ApnsClientBuilder setIdlePingInterval(final Duration idlePingInterval) {
490-
this.idlePingInterval = idlePingInterval;
489+
public ApnsClientBuilder setCloseAfterIdleDuration(final Duration closeAfterIdleDuration) {
490+
this.closeAfterIdleDuration = closeAfterIdleDuration;
491491
return this;
492492
}
493493

@@ -628,7 +628,7 @@ public ApnsClient build() throws SSLException {
628628
this.tokenExpiration,
629629
this.proxyHandlerFactory,
630630
this.connectionTimeout,
631-
this.idlePingInterval,
631+
this.closeAfterIdleDuration,
632632
this.gracefulShutdownTimeout,
633633
this.concurrentConnections,
634634
this.metricsListener,

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class ApnsClientConfiguration {
4646
private final Duration tokenExpiration;
4747
private final ProxyHandlerFactory proxyHandlerFactory;
4848
private final Duration connectionTimeout;
49-
private final Duration idlePingInterval;
49+
private final Duration closeAfterIdleDuration;
5050
private final Duration gracefulShutdownTimeout;
5151
private final int concurrentConnections;
5252
private final ApnsClientMetricsListener metricsListener;
@@ -59,7 +59,7 @@ public ApnsClientConfiguration(final InetSocketAddress apnsServerAddress,
5959
final Duration tokenExpiration,
6060
final ProxyHandlerFactory proxyHandlerFactory,
6161
final Duration connectionTimeout,
62-
final Duration idlePingInterval,
62+
final Duration closeAfterIdleDuration,
6363
final Duration gracefulShutdownTimeout,
6464
final int concurrentConnections,
6565
final ApnsClientMetricsListener metricsListener,
@@ -72,7 +72,7 @@ public ApnsClientConfiguration(final InetSocketAddress apnsServerAddress,
7272
this.tokenExpiration = tokenExpiration != null ? tokenExpiration : DEFAULT_TOKEN_EXPIRATION;
7373
this.proxyHandlerFactory = proxyHandlerFactory;
7474
this.connectionTimeout = connectionTimeout;
75-
this.idlePingInterval = idlePingInterval;
75+
this.closeAfterIdleDuration = closeAfterIdleDuration;
7676
this.gracefulShutdownTimeout = gracefulShutdownTimeout;
7777
this.concurrentConnections = concurrentConnections;
7878
this.metricsListener = metricsListener;
@@ -107,8 +107,8 @@ public Optional<Duration> getConnectionTimeout() {
107107
return Optional.ofNullable(connectionTimeout);
108108
}
109109

110-
public Duration getIdlePingInterval() {
111-
return idlePingInterval;
110+
public Duration getCloseAfterIdleDuration() {
111+
return closeAfterIdleDuration;
112112
}
113113

114114
public Optional<Duration> getGracefulShutdownTimeout() {

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

Lines changed: 4 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -38,22 +38,18 @@
3838
import io.netty.handler.timeout.IdleStateEvent;
3939
import io.netty.util.AsciiString;
4040
import io.netty.util.collection.IntObjectHashMap;
41-
import io.netty.util.concurrent.GenericFutureListener;
4241
import io.netty.util.concurrent.Promise;
4342
import io.netty.util.concurrent.PromiseCombiner;
44-
import io.netty.util.concurrent.ScheduledFuture;
4543
import org.slf4j.Logger;
4644
import org.slf4j.LoggerFactory;
4745

4846
import java.io.IOException;
4947
import java.nio.charset.StandardCharsets;
5048
import java.text.ParseException;
51-
import java.time.Duration;
5249
import java.util.Map;
5350
import java.util.Objects;
5451
import java.util.UUID;
5552
import java.util.concurrent.CompletableFuture;
56-
import java.util.concurrent.TimeUnit;
5753

5854
class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameListener, Http2Connection.Listener {
5955

@@ -65,9 +61,6 @@ class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameList
6561

6662
private final String authority;
6763

68-
private final Duration pingTimeout;
69-
private ScheduledFuture<?> pingTimeoutFuture;
70-
7164
private Throwable connectionErrorCause;
7265

7366
private static final AsciiString APNS_PATH_PREFIX = new AsciiString("/3/device/");
@@ -92,7 +85,6 @@ class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameList
9285
public static class ApnsClientHandlerBuilder extends AbstractHttp2ConnectionHandlerBuilder<ApnsClientHandler, ApnsClientHandlerBuilder> {
9386

9487
private String authority;
95-
private Duration idlePingInterval;
9688

9789
ApnsClientHandlerBuilder authority(final String authority) {
9890
this.authority = authority;
@@ -103,15 +95,6 @@ String authority() {
10395
return this.authority;
10496
}
10597

106-
Duration idlePingInterval() {
107-
return idlePingInterval;
108-
}
109-
110-
ApnsClientHandlerBuilder idlePingInterval(final Duration idlePingIntervalMillis) {
111-
this.idlePingInterval = idlePingIntervalMillis;
112-
return this;
113-
}
114-
11598
@Override
11699
public ApnsClientHandlerBuilder frameLogger(final Http2FrameLogger frameLogger) {
117100
return super.frameLogger(frameLogger);
@@ -136,7 +119,7 @@ protected boolean encoderEnforceMaxConcurrentStreams() {
136119
public ApnsClientHandler build(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings) {
137120
Objects.requireNonNull(this.authority(), "Authority must be set before building an ApnsClientHandler.");
138121

139-
final ApnsClientHandler handler = new ApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.idlePingInterval());
122+
final ApnsClientHandler handler = new ApnsClientHandler(decoder, encoder, initialSettings, this.authority());
140123
this.frameListener(handler);
141124
return handler;
142125
}
@@ -147,7 +130,7 @@ public ApnsClientHandler build() {
147130
}
148131
}
149132

150-
ApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final Duration idlePingInterval) {
133+
ApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority) {
151134
super(decoder, encoder, initialSettings);
152135

153136
this.authority = authority;
@@ -157,8 +140,6 @@ public ApnsClientHandler build() {
157140
this.streamErrorCausePropertyKey = this.connection().newKey();
158141

159142
this.connection().addListener(this);
160-
161-
this.pingTimeout = idlePingInterval.dividedBy(2);
162143
}
163144

164145
@Override
@@ -264,22 +245,8 @@ protected Http2Headers getHeadersForPushNotification(final ApnsPushNotification
264245
@Override
265246
public void userEventTriggered(final ChannelHandlerContext context, final Object event) throws Exception {
266247
if (event instanceof IdleStateEvent) {
267-
log.trace("Sending ping due to inactivity.");
268-
269-
this.encoder().writePing(context, false, System.currentTimeMillis(), context.newPromise()).addListener(
270-
(GenericFutureListener<ChannelFuture>) future -> {
271-
if (!future.isSuccess()) {
272-
log.debug("Failed to write PING frame.", future.cause());
273-
future.channel().close();
274-
}
275-
});
276-
277-
this.pingTimeoutFuture = context.channel().eventLoop().schedule(() -> {
278-
log.debug("Closing channel due to ping timeout.");
279-
context.channel().close();
280-
}, pingTimeout.toMillis(), TimeUnit.MILLISECONDS);
281-
282-
this.flush(context);
248+
log.debug("Closing idle channel.");
249+
context.close();
283250
}
284251

285252
super.userEventTriggered(context, event);
@@ -413,11 +380,6 @@ public void onPingRead(final ChannelHandlerContext ctx, final long pingData) {
413380

414381
@Override
415382
public void onPingAckRead(final ChannelHandlerContext context, final long pingData) {
416-
if (this.pingTimeoutFuture != null) {
417-
this.pingTimeoutFuture.cancel(false);
418-
} else {
419-
log.error("Received PING ACK, but no corresponding outbound PING found.");
420-
}
421383
}
422384

423385
@Override
@@ -508,10 +470,6 @@ protected void onConnectionError(final ChannelHandlerContext context, final bool
508470

509471
@Override
510472
public void channelInactive(final ChannelHandlerContext context) throws Exception {
511-
if (this.pingTimeoutFuture != null) {
512-
this.pingTimeoutFuture.cancel(false);
513-
}
514-
515473
for (final PushNotificationFuture<?, ?> future : this.unattachedResponsePromisesByStreamId.values()) {
516474
future.completeExceptionally(STREAM_CLOSED_BEFORE_REPLY_EXCEPTION);
517475
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,14 @@ public ApnsClientHandler build(final Http2ConnectionDecoder decoder, final Http2
8181
Objects.requireNonNull(this.signingKey(), "Signing key must be set before building a TokenAuthenticationApnsClientHandler.");
8282
Objects.requireNonNull(this.tokenExpiration(), "Token expiration duration must be set before building a TokenAuthenticationApnsClientHandler.");
8383

84-
final ApnsClientHandler handler = new TokenAuthenticationApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.idlePingInterval(), this.signingKey(), this.tokenExpiration());
84+
final ApnsClientHandler handler = new TokenAuthenticationApnsClientHandler(decoder, encoder, initialSettings, this.authority(), this.signingKey(), this.tokenExpiration());
8585
this.frameListener(handler);
8686
return handler;
8787
}
8888
}
8989

90-
protected TokenAuthenticationApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final Duration idlePingInterval, final ApnsSigningKey signingKey, final Duration tokenExpiration) {
91-
super(decoder, encoder, initialSettings, authority, idlePingInterval);
90+
protected TokenAuthenticationApnsClientHandler(final Http2ConnectionDecoder decoder, final Http2ConnectionEncoder encoder, final Http2Settings initialSettings, final String authority, final ApnsSigningKey signingKey, final Duration tokenExpiration) {
91+
super(decoder, encoder, initialSettings, authority);
9292

9393
this.signingKey = Objects.requireNonNull(signingKey, "Signing key must not be null for token-based client handlers.");
9494
this.tokenExpiration = Objects.requireNonNull(tokenExpiration, "Token expiration must not be null for token-based client handlers");

0 commit comments

Comments
 (0)