Validate that the response content-length header matches what the ser… #2957

millems · 2022-01-07T19:40:55Z

…vice actually returns.

Also update error messaging in the event of service-side connection closes, and add testing to protect these error messages against regression.

Bennett-Lynch · 2022-01-07T19:48:44Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+        Long contentLengthHeader = ctx.channel().attr(RESPONSE_CONTENT_LENGTH).get();
+        Long actualContentLength = ctx.channel().attr(RESPONSE_DATA_READ).get();


Can we consolidate this logic to a standalone handler, and that handler raises an exception if it detects this issue? The ResponseHandler would then propagate any channel exception.

Successes and errors are reported in multiple places in the netty client. I can see about how that works, but it's harder for me to guarantee that it will work because of how messed up our success/error reporting is.

Looking at it, I can already tell it might require me to rewrite our FullHttpResponse handling, since that currently handles does its own finalizeResponse call in parallel with this publisher to complete the future successfully...

Got it. Out of scope if it requires any significant refactoring. In general I would consider channel attribute usage a code smell and prefer to try to isolate behavior to standalone handlers, but understood if it's not feasible with the current setup.

That's good feedback. I'll see what I can do...

Yeah, it seems like FullResponseContentPublisher and PublisherAdapter do their own error handling, so I can't decorate them with something else and expect it to do anything. The onError just gets fired out to the customer's publisher and the netty client ignores it. I'd have to break up those two publishers to put their error handling downstream from the publishers themselves. It would be good to do that, but it's a significant refactor.

Bennett-Lynch · 2022-01-07T19:49:59Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+        return Optional.of(new IOException("Response had content-length of " + contentLengthHeader + " bytes, but only received "
+                                           + actualContentLength + " before the connection was closed."));


What if Transfer-Encoding: chunked is being used, but we don't receive the final (0 byte) chunk?

Also what happens if an HTTP/2 channel is closed without an endOfStream flag? Does Netty throw an exception?

That sounds like a different edge case we might need to add validation for. That's much rarer, since we rarely use chunked encoding. This particular code only goes into effect when the content-length is specified.

Yeah, maybe we also need to test the end-of-stream flag as well.

Bennett-Lynch · 2022-01-07T21:45:54Z

...-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelDiagnostics.java

+/**
+ * Diagnostic information that may be useful to help with debugging during error scenarios.
+ */
+@SdkInternalApi
+public class ChannelDiagnostics {


This is an interesting way to expose more helpful information. I like the idea. However, if it grows in size over time, we may want to consider only emitting it if, e.g., DEBUG logging is enabled.

Yeah, I agree we need to be careful about it.

Bennett-Lynch · 2022-01-07T21:48:22Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+                                                                            new FullResponseContentPublisher(channelContext,
+                                                                                                             bb, ef)));
+
+                Optional<IOException> contentLengthFailure = validateResponseContentLength(channelContext);


Nit: I think try/catch may be more conventional than Optional here.

Bennett-Lynch · 2022-01-07T21:49:25Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

            } finally {
                Optional.ofNullable(fullContent).ifPresent(ByteBuf::release);
            }
        }
    }

+    private Long responseContentLength(HttpResponse response) {
+        String length = response.headers().get("content-length");


Nit: io.netty.handler.codec.http.HttpHeaderNames.CONTENT_LENGTH

Bennett-Lynch · 2022-01-07T21:54:30Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+        return Optional.of(new IOException("Response had content-length of " + contentLengthHeader + " bytes, but only received "
+                                           + actualContentLength + " before the connection was closed."));


nit: "but only received X bytes before"

nit: should we emit a different/special message if we received 0 bytes?

nit: should we wrap the IOException inside of Netty's PrematureChannelClosureException?

It would need to be the other way around: IOException on the outside, so it's seen a retryable. PrematureChannelClosureException is a RuntimeException. Does that change your mind on it? Should I use that inside?

Doesn't our default retry logic check 1-level of causes?

What happens if Netty does actually throw a PrematureChannelClosureException itself? E.g., here. Do we retry on that? It seems like we would want to.

It does check 1 level of causes, but I was under the impression that the contract for ExecutableHttpRequest.call is that only IOException will be retried, not necessarily one level of exception. I don't know if I have the wrong impression, or if there's further wrapping that happens upstream before the retry logic that necessitates checking 1 level deeper.

Bennett-Lynch · 2022-01-19T00:56:51Z

...-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelDiagnostics.java

+    @Override
+    public String toString() {
+        return ToString.builder("ChannelDiagnostics")
+                       .add("creationTime", channelCreationTime)


Would channel age be helpful here? It may be able to be inferred by subtracting creation time from a log timestamp time, but it seems age may typically be more relevant than creation time.

+1 I can remove creationTime and replace it with channelAge. I'd avoid both for what you mention - redundant information.

Bennett-Lynch · 2022-01-19T01:01:35Z

...lient/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyRequestExecutor.java

-                    makeRequest();
+                try {
+                    configureChannel();
+                    if (tryConfigurePipeline()) {


Should we make tryConfigurePipeline throw directly? This would wrap all of its exceptions with Failed to initiate request to.... Or do you want to reserve that for less explicit failure cases?

Sorry, I'm not following.

I added this try/catch because exceptions raised within this runnable go unreported and the completable future returned to the customer never returns (likely causing a deadlock in the customer's code). This change could be in a separate commit, since it's unrelated/unneeded to this change.

I just added it here because I ran into this bug during implementation.

tryConfigurePipeline raises several exceptions itself (by also invoking handleFailure). I was just wondering if we should wrap all of them with this new logic for the sake of consistency (by changing tryConfigurePipeline to simply throw). But perhaps that just unnecessarily nests the more pertinent error message.

Ahh, I see. We can combine them, yeah. Good idea.

Bennett-Lynch · 2022-01-19T01:05:12Z

...-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelDiagnostics.java

+    }
+
+    @Override
+    public String toString() {


Should we include the Channel itself here? Primarily for remote address information.

Bennett-Lynch · 2022-01-19T01:08:56Z

...io-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java

+        if (isAcquireTimeoutException(originalCause)) {
+            return new Throwable(getMessageForAcquireTimeoutException(), originalCause);
+        } else if (isTooManyPendingAcquiresException(originalCause)) {
+            return new Throwable(getMessageForTooManyAcquireOperationsError(), originalCause);


Unrelated to this PR specifically but I don't think we should instantiate a literal Throwable here. Would be more conventional to use Exception or RuntimeException for the generic case.

Yeah, not sure why we use throwable

Bennett-Lynch · 2022-01-19T01:20:15Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+                    validateResponseContentLength(channelContext);
+                    finalizeResponse(requestContext, channelContext);


If validateResponseContentLength throws we do not call finalizeResponse. Likewise for streaming. Do we still release the channel in that scenario?

Yep, exceptionCaught closes and releases the channel.

Bennett-Lynch · 2022-01-19T01:33:32Z

...io-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtils.java

    private static final Logger log = Logger.loggerFor(NettyUtils.class);

    private NettyUtils() {
    }

+    public static Throwable decorateException(Channel channel, Throwable originalCause) {


Nit: We almost have enough exception-related logic in this class to consider a NettyExceptionUtils or similar.

Bennett-Lynch · 2022-01-19T01:39:07Z

...est/java/software/amazon/awssdk/http/nio/netty/fault/ServerConnectivityErrorMessageTest.java

+import software.amazon.awssdk.utils.AttributeMap;
+import software.amazon.awssdk.utils.Logger;
+
+@RunWith(Parameterized.class)


Nit: Consider JUnit 5.

Are we on junit 5 for sure now? I honestly need to learn it, if so.

Both 4 and 5 work fine, but IMO we should try to ensure newly created tests are on 5 so that we don't create more future migration work. JUnit 5 parameterization is also a lot cleaner IMO.

Updated it to junit 5. I don't like not being able to use the test parameter in the before-each statement, but otherwise it seems similar to me.

Bennett-Lynch · 2022-01-19T01:43:07Z

...est/java/software/amazon/awssdk/http/nio/netty/fault/ServerConnectivityErrorMessageTest.java

+        return Arrays.asList(new TestCase(CloseTime.DURING_INIT, "The connection was closed during the request."),
+                             new TestCase(CloseTime.BEFORE_SSL_HANDSHAKE, "The connection was closed during the request."),
+                             new TestCase(CloseTime.DURING_SSL_HANDSHAKE, "The connection was closed during the request."),
+                             new TestCase(CloseTime.BEFORE_REQUEST_PAYLOAD, "The connection was closed during the request."),
+                             new TestCase(CloseTime.DURING_REQUEST_PAYLOAD, "The connection was closed during the request."),
+                             new TestCase(CloseTime.BEFORE_RESPONSE_HEADERS, "The connection was closed during the request."),
+                             new TestCase(CloseTime.BEFORE_RESPONSE_PAYLOAD, "Response had content-length"),
+                             new TestCase(CloseTime.DURING_RESPONSE_PAYLOAD, "Response had content-length"));


Awesome test cases. Can we come up with more distinctive wording for the first 6 test cases, or out of scope of this PR?

Do you have an example what you mean?

Something like this (numbers are not aligned to test cases):

The connection was closed while it was being initialized.

The connection was closed during the TLS handshake process.

The connection was closed before the request could be sent.

The connection was closed while sending the request body.

The connection was closed while waiting to receive a response.

The connection was closed while receiving the response body.

This type of wording could greatly help debugging certain failure scenarios.

The test case already provides something similar through its toString. I feel like that should be enough without needing to maintain a string description.

I meant can we change the user-facing wording used in the actual exception? I agree the test case enums are fine.

Gotcha, we can talk offline. I tried super hard to do that originally (that's why these test cases are structured like that), and it just doesn't work like that. The client thinks it's waiting for a response when the service hasn't even finished the SSL handshake.

Bennett-Lynch · 2022-01-19T01:45:19Z

...est/java/software/amazon/awssdk/http/nio/netty/fault/ServerConnectivityErrorMessageTest.java

+    @Test
+    public void closeTimeHasCorrectMessage() {
+        server.closeTime = testCase.closeTime;
+        assertThat(captureException()).hasMessageContaining(testCase.errorMessage);


Nit: Consider assertThatThrownBy.

I didn't want the assertions running against the Error throwables that could be raised by the logic in captureException.

Bennett-Lynch · 2022-01-19T01:46:12Z

...est/java/software/amazon/awssdk/http/nio/netty/fault/ServerConnectivityErrorMessageTest.java

+
+    private static class TestCase {
+        private CloseTime closeTime;
+        private String errorMessage;


Nit: Maybe errorSubstring

Bennett-Lynch · 2022-01-19T19:28:57Z

...-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ChannelDiagnostics.java

+    public String toString() {
+        return ToString.builder("ChannelDiagnostics")
+                       .add("channel", channel)
+                       .add("channelAge", Duration.between(channelCreationTime, Instant.now()).getSeconds() + " seconds")


I would prefer to just use the default Duration#toString format, even if it's not the prettiest. Flooring to seconds may lose a lot of granularity.

Fair enough!

…vice actually returns. Also update error messaging in the event of service-side connection closes, and add testing to protect these error messages against regression.

sonarqubecloud · 2022-01-20T00:59:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

90.1% Coverage
0.0% Duplication

… the service actually returns. (#2957)" This reverts commit f6fe8da.

…vice actually returns. (#2957) This reverts commit 6d93c8e.

* Validate that the response content-length header matches what the service actually returns. (#2957) This reverts commit 6d93c8e. * Do not validate content-length for 304 and HEAD responses.

millems requested a review from a team as a code owner January 7, 2022 19:40

Bennett-Lynch reviewed Jan 7, 2022

View reviewed changes

millems force-pushed the millem/verify-response-content-length branch 2 times, most recently from da0b052 to c832143 Compare January 7, 2022 22:43

millems force-pushed the millem/verify-response-content-length branch 6 times, most recently from 26a01a3 to cd5c184 Compare January 19, 2022 00:20

Bennett-Lynch reviewed Jan 19, 2022

View reviewed changes

millems force-pushed the millem/verify-response-content-length branch 2 times, most recently from be87573 to d6754fe Compare January 19, 2022 19:18

Bennett-Lynch reviewed Jan 19, 2022

View reviewed changes

millems force-pushed the millem/verify-response-content-length branch 2 times, most recently from b116a36 to 8b63113 Compare January 19, 2022 19:47

Validate that the response content-length header matches what the ser…

fe78581

…vice actually returns. Also update error messaging in the event of service-side connection closes, and add testing to protect these error messages against regression.

millems force-pushed the millem/verify-response-content-length branch from 8b63113 to fe78581 Compare January 19, 2022 19:50

Bennett-Lynch approved these changes Jan 19, 2022

View reviewed changes

millems enabled auto-merge (squash) January 19, 2022 23:08

millems added 2 commits January 19, 2022 15:09

Merge branch 'master' into millem/verify-response-content-length

aad4b40

Merge branch 'master' into millem/verify-response-content-length

4363f88

millems merged commit f6fe8da into master Jan 20, 2022

aws-sdk-java-automation pushed a commit that referenced this pull request Jan 20, 2022

Revert "Validate that the response content-length header matches what…

6d93c8e

… the service actually returns. (#2957)" This reverts commit f6fe8da.

millems added a commit that referenced this pull request Jan 21, 2022

Validate that the response content-length header matches what the ser…

1274717

…vice actually returns. (#2957) This reverts commit 6d93c8e.

millems mentioned this pull request Jan 21, 2022

Revert the rollback of #2957 #2982

Merged

millems deleted the millem/verify-response-content-length branch October 19, 2022 19:36

		Long contentLengthHeader = ctx.channel().attr(RESPONSE_CONTENT_LENGTH).get();
		Long actualContentLength = ctx.channel().attr(RESPONSE_DATA_READ).get();

		return Optional.of(new IOException("Response had content-length of " + contentLengthHeader + " bytes, but only received "
		+ actualContentLength + " before the connection was closed."));

		validateResponseContentLength(channelContext);
		finalizeResponse(requestContext, channelContext);

Validate that the response content-length header matches what the ser… #2957

Validate that the response content-length header matches what the ser… #2957

Uh oh!

Conversation

millems commented Jan 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bennett-Lynch Jan 19, 2022 •

edited

Loading

Bennett-Lynch Jan 19, 2022 •

edited

Loading