Skip to content
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.

Sign up for GitHub

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

Don't treat internal server errors as write exceptions #576

Merged
merged 1 commit into from
Apr 1, 2018
+9 −139
Merged

Don't treat internal server errors as write exceptions #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 7 additions & 25 deletions pushy/src/main/java/com/turo/pushy/apns/ApnsClientHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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.");
}
}
}
Expand All @@ -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) {
Expand Down
50 changes: 0 additions & 50 deletions pushy/src/main/java/com/turo/pushy/apns/ApnsServerException.java
View file
Open in desktop

This file was deleted.

18 changes: 1 addition & 17 deletions pushy/src/main/java/com/turo/pushy/apns/server/MockApnsServerHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
46 changes: 0 additions & 46 deletions pushy/src/test/java/com/turo/pushy/apns/ApnsClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PushNotificationResponse<SimpleApnsPushNotification>> 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 {
Expand Down
2 changes: 1 addition & 1 deletion pushy/src/test/java/com/turo/pushy/apns/server/MockApnsServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public void handlePushNotification(final Http2Headers headers, final ByteBuf pay
final Future<PushNotificationResponse<SimpleApnsPushNotification>> sendFuture =
client.sendNotification(pushNotification).await();

assertFalse(sendFuture.isSuccess());
assertTrue(sendFuture.isSuccess());

listener.waitForNonZeroRejectedNotifications();

Expand Down