From 164cb9efe203dabaccb762fb4a8b79bc128e0e6a Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Wed, 17 Jan 2018 22:14:41 -0500 Subject: [PATCH] Don't treat internal server errors as write exceptions. --- .../turo/pushy/apns/ApnsClientHandler.java | 32 +++--------- .../turo/pushy/apns/ApnsServerException.java | 50 ------------------- .../apns/server/MockApnsServerHandler.java | 18 +------ .../com/turo/pushy/apns/ApnsClientTest.java | 46 ----------------- .../pushy/apns/server/MockApnsServerTest.java | 2 +- 5 files changed, 9 insertions(+), 139 deletions(-) delete mode 100644 pushy/src/main/java/com/turo/pushy/apns/ApnsServerException.java diff --git a/pushy/src/main/java/com/turo/pushy/apns/ApnsClientHandler.java b/pushy/src/main/java/com/turo/pushy/apns/ApnsClientHandler.java index 7c7dc881d..53fdc7388 100644 --- a/pushy/src/main/java/com/turo/pushy/apns/ApnsClientHandler.java +++ b/pushy/src/main/java/com/turo/pushy/apns/ApnsClientHandler.java @@ -76,13 +76,6 @@ class ApnsClientHandler extends Http2ConnectionHandler implements Http2FrameList private static final IOException STREAM_CLOSED_BEFORE_REPLY_EXCEPTION = new IOException("Stream closed before a reply was received"); - private static final ApnsServerException APNS_SERVER_EXCEPTION = new ApnsServerException() { - @Override - public Throwable fillInStackTrace() { - return this; - } - }; - private static final Gson GSON = new GsonBuilder() .registerTypeAdapter(Date.class, new DateAsTimeSinceEpochTypeAdapter(TimeUnit.MILLISECONDS)) .create(); @@ -340,17 +333,11 @@ private void handleEndOfStream(final ChannelHandlerContext context, final Http2S responsePromise.trySuccess(new SimplePushNotificationResponse<>(responsePromise.getPushNotification(), true, getApnsIdFromHeaders(headers), null, null)); } else { - if (HttpResponseStatus.INTERNAL_SERVER_ERROR.equals(status)) { - log.warn("APNs server reported an internal error when sending {}.", pushNotification); - responsePromise.tryFailure(APNS_SERVER_EXCEPTION); - context.channel().close(); + if (data != null) { + final ErrorResponse errorResponse = GSON.fromJson(data.toString(StandardCharsets.UTF_8), ErrorResponse.class); + this.handleErrorResponse(context, stream.id(), headers, pushNotification, errorResponse); } else { - if (data != null) { - final ErrorResponse errorResponse = GSON.fromJson(data.toString(StandardCharsets.UTF_8), ErrorResponse.class); - this.handleErrorResponse(context, stream.id(), headers, pushNotification, errorResponse); - } else { - log.warn("Gateway sent an end-of-stream HEADERS frame for an unsuccessful notification."); - } + log.warn("Gateway sent an end-of-stream HEADERS frame for an unsuccessful notification."); } } } @@ -361,14 +348,9 @@ protected void handleErrorResponse(final ChannelHandlerContext context, final in final HttpResponseStatus status = HttpResponseStatus.parseLine(headers.status()); - if (HttpResponseStatus.INTERNAL_SERVER_ERROR.equals(status)) { - log.warn("APNs server reported an internal error when sending {}.", pushNotification); - responsePromise.tryFailure(new ApnsServerException(GSON.toJson(errorResponse))); - } else { - responsePromise.trySuccess(new SimplePushNotificationResponse<>(responsePromise.getPushNotification(), - HttpResponseStatus.OK.equals(status), getApnsIdFromHeaders(headers), errorResponse.getReason(), - errorResponse.getTimestamp())); - } + responsePromise.trySuccess(new SimplePushNotificationResponse<>(responsePromise.getPushNotification(), + HttpResponseStatus.OK.equals(status), getApnsIdFromHeaders(headers), errorResponse.getReason(), + errorResponse.getTimestamp())); } private static UUID getApnsIdFromHeaders(final Http2Headers headers) { diff --git a/pushy/src/main/java/com/turo/pushy/apns/ApnsServerException.java b/pushy/src/main/java/com/turo/pushy/apns/ApnsServerException.java deleted file mode 100644 index 354d8c348..000000000 --- a/pushy/src/main/java/com/turo/pushy/apns/ApnsServerException.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright (c) 2013-2017 Turo - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - * THE SOFTWARE. - */ - -package com.turo.pushy.apns; - -/** - * An exception that indicates that a push notification could not be sent due to an upstream server error. Server errors - * should be considered temporary failures, and callers should attempt to send the notification again later. - * - * @author Jon Chambers - * - */ -public class ApnsServerException extends Exception { - private static final long serialVersionUID = 1L; - - /** - * Constructs a new APNs server exception. - */ - public ApnsServerException() { - super(); - } - - /** - * Constructs a new APNs server exception with the given message. - * - * @param message a message from the server (if any) explaining the error; may be {@code null} - */ - public ApnsServerException(final String message) { - super(message); - } -} diff --git a/pushy/src/main/java/com/turo/pushy/apns/server/MockApnsServerHandler.java b/pushy/src/main/java/com/turo/pushy/apns/server/MockApnsServerHandler.java index 98d1a4a10..81e93608f 100644 --- a/pushy/src/main/java/com/turo/pushy/apns/server/MockApnsServerHandler.java +++ b/pushy/src/main/java/com/turo/pushy/apns/server/MockApnsServerHandler.java @@ -138,12 +138,6 @@ Date getTimestamp() { } } - private static class InternalServerErrorResponse extends ApnsResponse { - private InternalServerErrorResponse(final int streamId, final UUID apnsId) { - super(streamId, apnsId); - } - } - @SuppressWarnings("unused") private static class ErrorPayload { private final String reason; @@ -290,7 +284,7 @@ private void handleEndOfStream(final ChannelHandlerContext context, final Http2S this.write(context, new RejectNotificationResponse(stream.id(), apnsId, e.getRejectionReason(), deviceTokenExpirationTimestamp), writePromise); this.listener.handlePushNotificationRejected(headers, payload, e.getRejectionReason(), deviceTokenExpirationTimestamp); } catch (final Exception e) { - this.write(context, new InternalServerErrorResponse(stream.id(), apnsId), writePromise); + this.write(context, new RejectNotificationResponse(stream.id(), apnsId, RejectionReason.INTERNAL_SERVER_ERROR, null), writePromise); this.listener.handlePushNotificationRejected(headers, payload, RejectionReason.INTERNAL_SERVER_ERROR, null); } finally { if (stream.getProperty(this.payloadPropertyKey) != null) { @@ -341,16 +335,6 @@ public void write(final ChannelHandlerContext context, final Object message, fin promiseCombiner.finish(writePromise); log.trace("Rejected push notification on stream {}: {}", rejectNotificationResponse.getStreamId(), rejectNotificationResponse.getErrorReason()); - } else if (message instanceof InternalServerErrorResponse) { - final InternalServerErrorResponse internalServerErrorResponse = (InternalServerErrorResponse) message; - - final Http2Headers headers = new DefaultHttp2Headers() - .status(HttpResponseStatus.INTERNAL_SERVER_ERROR.codeAsText()) - .add(APNS_ID_HEADER, FastUUID.toString(internalServerErrorResponse.getApnsId())); - - this.encoder().writeHeaders(context, internalServerErrorResponse.getStreamId(), headers, 0, true, writePromise); - - log.trace("Encountered an internal error on stream {}", internalServerErrorResponse.getStreamId()); } else { context.write(message, writePromise); } diff --git a/pushy/src/test/java/com/turo/pushy/apns/ApnsClientTest.java b/pushy/src/test/java/com/turo/pushy/apns/ApnsClientTest.java index fc85de0ed..9a9143a65 100644 --- a/pushy/src/test/java/com/turo/pushy/apns/ApnsClientTest.java +++ b/pushy/src/test/java/com/turo/pushy/apns/ApnsClientTest.java @@ -502,52 +502,6 @@ public void handlePushNotification(final Http2Headers headers, final ByteBuf pay } } - @Test - @Parameters({"true", "false"}) - public void testSendNotificationWithInternalServerError(final boolean useTokenAuthentication) throws Exception { - final PushNotificationHandlerFactory handlerFactory = new PushNotificationHandlerFactory() { - @Override - public PushNotificationHandler buildHandler(final SSLSession sslSession) { - return new PushNotificationHandler() { - @Override - public void handlePushNotification(final Http2Headers headers, final ByteBuf payload) { - throw new RuntimeException("I am a terrible server."); - } - }; - } - }; - - final MockApnsServer server = this.buildServer(handlerFactory); - - final TestMetricsListener metricsListener = new TestMetricsListener(); - final ApnsClient client = useTokenAuthentication ? - this.buildTokenAuthenticationClient(metricsListener) : this.buildTlsAuthenticationClient(metricsListener); - - try { - server.start(PORT).await(); - - final SimpleApnsPushNotification pushNotification = - new SimpleApnsPushNotification(DEVICE_TOKEN, TOPIC, PAYLOAD); - - final Future> future = - client.sendNotification(pushNotification).await(); - - assertFalse("Internal server errors should be treated as write failures rather than deliberate rejections.", - future.isSuccess()); - - assertTrue("Internal server errors should be reported to callers as ApnsServerExceptions.", - future.cause() instanceof ApnsServerException); - - client.sendNotification(pushNotification).await(); - - assertEquals("Connections should be replaced after receiving an InternalServerError.", - 1, metricsListener.getConnectionsRemoved().get()); - } finally { - client.close().await(); - server.shutdown().await(); - } - } - @Test @Parameters({"true", "false"}) public void testAcceptedNotificationAndAddedConnectionMetrics(final boolean useTokenAuthentication) throws Exception { diff --git a/pushy/src/test/java/com/turo/pushy/apns/server/MockApnsServerTest.java b/pushy/src/test/java/com/turo/pushy/apns/server/MockApnsServerTest.java index 1cfbf3692..8f43f5686 100644 --- a/pushy/src/test/java/com/turo/pushy/apns/server/MockApnsServerTest.java +++ b/pushy/src/test/java/com/turo/pushy/apns/server/MockApnsServerTest.java @@ -285,7 +285,7 @@ public void handlePushNotification(final Http2Headers headers, final ByteBuf pay final Future> sendFuture = client.sendNotification(pushNotification).await(); - assertFalse(sendFuture.isSuccess()); + assertTrue(sendFuture.isSuccess()); listener.waitForNonZeroRejectedNotifications();