Skip to content

Commit

Permalink
Include ConnectTimeoutException in timeout exceptions for rules
Browse files Browse the repository at this point in the history
Motivation:

`CircuitBreakerRuleBuilder.onTimeException()` and
`RetryRuleBuilder.onTimeoutException()` only handle Armeria's
`com.linecorp.armeria.common.TimeoutException`. Howerver, they do not
treat Netty`s `ConnectTimeoutException` as a timeout.

Modifitions:

- Treat Netty's `ConnectTimeoutException` and
  `ProxyConnectException("timeout")` as a timeout for rules.

Result:

`CircuitBreakerRuleBuilder.onTimeException()` and `RetryRuleBuilder.onTimeoutException()`
now work when a connection or proxy establishment is timed out.
  • Loading branch information
ikhoon committed Feb 18, 2025
1 parent 533f798 commit f4b22dd
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import com.linecorp.armeria.common.TimeoutException;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.annotation.UnstableApi;
import com.linecorp.armeria.internal.common.TimeoutExceptionPredicate;

/**
* A skeletal builder implementation for {@link RetryRule}, {@link RetryRuleWithContent},
Expand Down Expand Up @@ -257,8 +258,7 @@ public SELF onTimeoutException() {
if (ctx.isTimedOut()) {
return true;
}
return ex instanceof TimeoutException ||
ex instanceof UnprocessedRequestException && ex.getCause() instanceof TimeoutException;
return TimeoutExceptionPredicate.isTimeoutException(ex);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.common.util.TimeoutMode;
import com.linecorp.armeria.internal.common.RequestContextUtil;
import com.linecorp.armeria.internal.common.TimeoutExceptionPredicate;
import com.linecorp.armeria.server.ServiceRequestContext;

import io.netty.util.Attribute;
Expand Down Expand Up @@ -525,8 +526,7 @@ default boolean isTimedOut() {
return true;
}
final Throwable cause = cancellationCause();
return cause instanceof TimeoutException ||
cause instanceof UnprocessedRequestException && cause.getCause() instanceof TimeoutException;
return TimeoutExceptionPredicate.isTimeoutException(cause);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright 2025 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.internal.common;

import com.linecorp.armeria.client.UnprocessedRequestException;
import com.linecorp.armeria.common.TimeoutException;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.internal.client.dns.DnsUtil;

import io.netty.channel.ConnectTimeoutException;
import io.netty.handler.proxy.ProxyConnectException;

public final class TimeoutExceptionPredicate {

public static boolean isTimeoutException(@Nullable Throwable cause) {
if (cause == null) {
return false;
}
cause = peel(cause);

if (cause instanceof TimeoutException) {
// Armeria timeout.
return true;
}
if (cause instanceof ConnectTimeoutException) {
// A connection level timeout.
return true;
}

if (cause instanceof ProxyConnectException) {
// https://github.com/netty/netty/blob/0138f2335593d50e3eb1d8e0e97d3e8438f7a74d/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java#L201
final String message = cause.getMessage();
// TODO(ikhoon): Consider sending a PR to Netty to add a dedicated exception type for this.
if (message != null && message.contains("timeout")) {
return true;
}
}
// A DNS level timeout.
return DnsUtil.isDnsQueryTimedOut(cause);
}

private static Throwable peel(Throwable cause) {
cause = Exceptions.peel(cause);
if (cause instanceof UnprocessedRequestException) {
cause = cause.getCause();
}
assert cause != null;
return cause;
}

private TimeoutExceptionPredicate() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2025 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.internal.common;

import static com.linecorp.armeria.internal.common.TimeoutExceptionPredicate.isTimeoutException;
import static org.assertj.core.api.Assertions.assertThat;

import org.junit.jupiter.api.Test;

import com.linecorp.armeria.client.DnsTimeoutException;
import com.linecorp.armeria.client.UnprocessedRequestException;
import com.linecorp.armeria.internal.testing.AnticipatedException;
import com.linecorp.armeria.server.RequestTimeoutException;

import io.netty.channel.ConnectTimeoutException;
import io.netty.handler.proxy.ProxyConnectException;

class TimeoutExceptionPredicateTest {

@Test
void testTimeoutException() {
assertThat(isTimeoutException(new ConnectTimeoutException())).isTrue();
assertThat(isTimeoutException(RequestTimeoutException.get())).isTrue();
assertThat(isTimeoutException(new DnsTimeoutException("test"))).isTrue();
assertThat(isTimeoutException(new ProxyConnectException("timeout"))).isTrue();
}

@Test
void unwrapUnprocessedException() {
UnprocessedRequestException unprocessedException =
UnprocessedRequestException.of(new ConnectTimeoutException());
assertThat(isTimeoutException(unprocessedException)).isTrue();

unprocessedException = UnprocessedRequestException.of(new ProxyConnectException("... timeout"));
assertThat(isTimeoutException(unprocessedException)).isTrue();

unprocessedException = UnprocessedRequestException.of(new AnticipatedException());
assertThat(isTimeoutException(unprocessedException)).isFalse();
}
}

0 comments on commit f4b22dd

Please sign in to comment.