Adjust parent types for exceptions #1924

idelpivnitskiy · 2021-10-29T22:26:55Z

Motivation:

After revisiting all existing exception types, we found that some of
them use incorrect parent type.

Modifications:

Use more appropriate parent type for existing exceptions:
- ConnectionRejectedException extend RuntimeException -> SocketException;
- NoAvailableHostException extend RuntimeException -> IOException;
- DuplicateAttributeException extend RuntimeException -> IllegalStateException;
- TerminateRepeatException extend Exception -> RuntimeException;
- ClosedServiceDiscovererException extend RuntimeException -> ClosedChannelException;
- UnsupportedHttpChunkException extend SerializationException -> IllegalArgumentException;
Regenerate serialVersionUID for updated exception classes;
Use StacklessConnectionRejectedException for
RoundRobinLoadBalancer;
Mark MessageEncodingException as @Deprecated (was missed during rework of
serialization API);
Remove unused ctor from ResponseTimeoutTest;

Result:

Exceptions have more appropriate parent types.

Motivation: After revisiting all existing exception types, we found that some of them use incorrect parent type. Modifications: - Use more appropriate parent type for existing exceptions; - Regenerate `serialVersionUID` for updated exception classes; - Fix how the message is generated for `MessageEncodingException`; - Change exception type thrown by `DefaultMultiAddressUrlHttpClientBuilder` to `MalformedURLException` when request target is not in absolute-form; - Remove unused ctor from `ResponseTimeoutTest`; - Use `StacklessConnectionRejectedException` for `RoundRobinLoadBalancer`; Result: Exceptions have more appropriate parent types.

idelpivnitskiy

I don't expect it to be an impactful breaking change because most of the users use our types instead of parent types. Should be an appropriate change for 0.42 release, wdyt?

idelpivnitskiy · 2021-10-29T22:31:25Z

servicetalk-client-api/src/main/java/io/servicetalk/client/api/ConnectionRejectedException.java

 /**
 * Thrown when a newly created connection was rejected.
 */
-public final class ConnectionRejectedException extends RuntimeException implements RetryableException {
-    private static final long serialVersionUID = -2573006397795678870L;
+public class ConnectionRejectedException extends SocketException implements RetryableException {


Debated between ConnectException and SocketException. Based on their javadoc, decided to go with SocketException because technically the connection was already established, but there was a problem "accessing" it due to a selector or a host becoming unavailable.

idelpivnitskiy · 2021-10-29T22:32:41Z

servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RepeatStrategies.java

@@ -40,8 +40,8 @@
    /**
     * An {@link Exception} instance used to indicate termination of repeats.
     */
-    public static final class TerminateRepeatException extends Exception {
-        private static final long serialVersionUID = -1725458427890873886L;
+    public static final class TerminateRepeatException extends RuntimeException {


The decision to throw this exception happens at runtime

idelpivnitskiy · 2021-10-29T22:33:33Z

servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/MessageEncodingException.java

 */
+@Deprecated
 public final class MessageEncodingException extends RuntimeException {


ProtoBufSerializationProviderBuilder was deprecated only in the main branch, we don't need to cherry-pick this into 0.41

...rotobuf/src/main/java/io/servicetalk/grpc/protobuf/ProtoBufSerializationProviderBuilder.java

idelpivnitskiy · 2021-10-29T22:35:47Z

servicetalk-http-api/src/main/java/io/servicetalk/http/api/UnsupportedHttpChunkException.java

 import javax.annotation.Nullable;

-final class UnsupportedHttpChunkException extends SerializationException {
+final class UnsupportedHttpChunkException extends IllegalArgumentException {


The change from IllegalArgumentException to SerializationException was made only in the main branch in #1673. This is not related to serialization and more about what payload body object users passed to an HTTP message. Reverting back to IllegalArgumentException.
cc @Scottmitch

idelpivnitskiy · 2021-10-29T22:36:39Z

...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java

+            return defer(() -> {
+                try {
+                    return selectClient(request).request(strategy, request).subscribeShareContext();
+                } catch (Throwable t) {


The try-catch here is necessary anyway, because methods that return async sources are not expected to throw.

tkountis · 2021-11-09T11:57:07Z

servicetalk-client-api/src/main/java/io/servicetalk/client/api/NoAvailableHostException.java

 /**
 * Thrown when no host is available but at least one is required.
 */
-public class NoAvailableHostException extends RuntimeException implements RetryableException {
-    private static final long serialVersionUID = 5340791072245425967L;
+public class NoAvailableHostException extends SocketException implements RetryableException {


Is it a "SocketException"? I would rather keep it a runtime one or associate with a LB/SD parent type.

The intend was to make it IOException because it prevents a request execution due to unavailability to execute an IO operation. During offline discussion, @bondolo recommended a more specific subtype - SocketException. I'm agreed with that proposal because it notifies that there are no sockets available at this point of time.

Don't see a reason to block the change but I don't agree that this is a good correlation. I agree that IOException is a valid super type, but SocketException is clearly doc'ed to do with failure to access or create a socket. In our case this is not even related to a connection but rather to host-finding.

Ok, I was reading "failure to access a socket" as "no hosts -> no access" 😄
Changing to IOException: c2b41cf

bondolo · 2021-11-09T20:04:36Z

...p-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java

@@ -63,6 +64,9 @@
 * A builder of {@link StreamingHttpClient} instances which have a capacity to call any server based on the parsed
 * absolute-form URL address information from each {@link StreamingHttpRequest}.
 * <p>
+ * If {@link HttpRequestMetaData#requestTarget()} is not an absolute-form URL, a {@link MalformedURLException} will be


Possibly just a personal preference but I usually prefer URISyntaxException to MalformedURLException because the later often gets handled as a generic IOException by lazy people who hate checked exceptions.

Hm... Haven't thought about URISyntaxException. Reading its javadoc, it's hard to say that /path is expected to trigger URISyntaxException. Description of MalformedURLException is better aligned with multi-address client requirements. What others think?

I'm agreed that users usually catch broad IOException. A good thing with Reactive Streams is that you don't have to throw and try-catch checked exceptions, everything is propagated through onError as Throwable.
BlockingHttpClient signature defines a request method as throws Exception.

I've opened a separate PR #1955 for this change to make it move visible in release notes

idelpivnitskiy added the breaking-change PR with breaking changes, removed APIs or other binary incompatibilities. label Oct 29, 2021

idelpivnitskiy requested review from bondolo, chemicL, tkountis and Scottmitch October 29, 2021 22:26

idelpivnitskiy self-assigned this Oct 29, 2021

idelpivnitskiy commented Oct 29, 2021

View reviewed changes

idelpivnitskiy added 2 commits November 4, 2021 15:30

Merge remote-tracking branch 'upstream/main' into exceptions

10e7183

Revert changes for ProtoBufSerializationProviderBuilder

53550fe

idelpivnitskiy mentioned this pull request Nov 8, 2021

MessageEncodingException should take an encoding name only #1940

Merged

Merge remote-tracking branch 'upstream/main' into exceptions

ef60d6a

tkountis reviewed Nov 9, 2021

View reviewed changes

bondolo approved these changes Nov 9, 2021

View reviewed changes

idelpivnitskiy added 2 commits November 13, 2021 00:03

NoAvailableHostException extends IOException

c2b41cf

Revert changes for multi-address client (will submit a separate PR)

2a7a255

idelpivnitskiy merged commit e9d69fa into apple:main Nov 15, 2021

idelpivnitskiy deleted the exceptions branch November 15, 2021 16:19

idelpivnitskiy mentioned this pull request Nov 15, 2021

Multi-address client: throw MalformedURLException instead of IllegalArgumentException #1955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust parent types for exceptions #1924

Adjust parent types for exceptions #1924

idelpivnitskiy commented Oct 29, 2021 •

edited

Loading

idelpivnitskiy left a comment

idelpivnitskiy Oct 29, 2021 •

edited

Loading

idelpivnitskiy Oct 29, 2021

idelpivnitskiy Oct 29, 2021

idelpivnitskiy Oct 29, 2021 •

edited

Loading

idelpivnitskiy Oct 29, 2021

tkountis Nov 9, 2021

idelpivnitskiy Nov 9, 2021

tkountis Nov 12, 2021

idelpivnitskiy Nov 15, 2021

bondolo Nov 9, 2021

idelpivnitskiy Nov 9, 2021

idelpivnitskiy Nov 15, 2021

Adjust parent types for exceptions #1924

Adjust parent types for exceptions #1924

Conversation

idelpivnitskiy commented Oct 29, 2021 • edited Loading

idelpivnitskiy left a comment

Choose a reason for hiding this comment

idelpivnitskiy Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy commented Oct 29, 2021 •

edited

Loading

idelpivnitskiy Oct 29, 2021 •

edited

Loading

idelpivnitskiy Oct 29, 2021 •

edited

Loading