fix(rt): add special-casing for CrtHttpEngine `aws-chunked` requests #760

lauzadis · 2022-12-09T19:53:38Z

CRT requires aws-chunked requests to use a specific API instead of a normal streaming request body. This PR adds a special-case to the CRT engine, which will use this new API instead of sending an aws-chunked body.

Issue #

#759

Description of changes

Currently aws-chunked requests do not work when using the CrtHttpEngine.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ianbotsf · 2022-12-09T22:51:02Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

@@ -100,6 +103,40 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE
        val stream = conn.makeRequest(engineRequest, respHandler)
        stream.activate()

+        if (request.isChunked) {
+            val bytesToRead = 65536L


Question: Why is this the right number? I suggest making this a file-level private const val and adding a comment or KDoc describing how this value was derived.

There is no "right" number here, it is just the size of the buffer used to move data from the aws-chunked body to the CRT engine. I can clarify that and make it a private const val.

ianbotsf · 2022-12-09T23:01:46Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+                    val source = (request.body as HttpBody.SourceContent).readFrom()
+                    val nextBuffer = SdkBuffer()
+                    var buffer = SdkBuffer()
+
+                    while (true) {
+                        source.read(buffer, bytesToRead)
+
+                        // read another set of bytes into a second buffer -- this is how we can "peek" to decide if this is the final chunk or not
+                        val isFinalChunk = source.read(nextBuffer, bytesToRead) == -1L
+
+                        stream.writeChunk(buffer.readToByteArray(), isFinalChunk)
+
+                        if (isFinalChunk) break else buffer = nextBuffer
+                    }


Question: Am I reading this correctly that the first writeChunk will get 64K, subsequent writeChunks will get 128K (read, swap, read again), and the final writeChunk will get whatever's left? If so, does the initial chunk's smaller size matter?

That's right, and the sizes are all arbitrary. We should select a size that we are comfortable loading into memory all-at-once, and I think 128K is fine.

ianbotsf · 2022-12-09T23:04:56Z

...s/http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/RequestUtil.kt

            val source = body.readFrom()
            callContext.job.invokeOnCompletion {
                source.close()
            }
            SdkSourceBodyStream(source)
        }
+        else -> null


Question: What could else be if not one of the above cases? Should the stream be null or should we actually throw an error?

Probably throw an error. I wish there was a better way to format this when block, ideally it would enumerate all the HttpBody types and nothing else, but I needed access to the HttpRequest to call isChunked

this should throw an error on the else clause now

I refactored to get rid of the dangling else:

val bodyStream = if (isChunked) null else when (body) { is HttpBody.Empty -> null ... }

aajtodd · 2022-12-12T13:41:21Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

@@ -100,6 +103,40 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE
        val stream = conn.makeRequest(engineRequest, respHandler)
        stream.activate()

+        if (request.isChunked) {


This doesn't seem like it belongs here, this chunked send logic should probably be moved to it's own function. (making it's own function and taking in a Stream will also allow you to unit test it independently)

question: This whole function blocks the thread until the entire chunked body is sent, is that the way CRT expects this?
* makeRequest() -> stream.activate() -> send entire chunked body?

fix: Assuming this is correct to block until the body is sent we should move the entirety of this work to SdkDispatchers.IO

stream.activate() if (request.isChunked) { withContext(SdkDispatchers.IO) { sendChunkedBody(...) } } ... internal fun sendChunkedBody(...) { // your new code here }

comment/no-op: This is unfortunate that toCrtRequest() doesn't actually do the complete conversion anymore. I suppose this is more CRTs fault than anything because of how they've structured the APIs. I don't have a better suggestion at the moment but this feels "wrong" that we call makeRequest and then have to separately check for chunked bodies and handle them differently.

Discussed offline -- since this writeChunk method is only intended for HTTP1.1, we do not have to consider doing this in a non-blocking manner because there are no duplex streams.

I updated to use SdkDispatchers.IO, refactored to its own function and added some tests for it.

aajtodd · 2022-12-12T13:57:00Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+
+                    while (!chan.isClosedForRead) {
+                        val bytesRead = chan.read(buffer, bytesToRead)
+                        check(bytesRead != -1L) { "unexpected exhaustion of channel" }


fix: This isn't right, we could check isClosedForRead and then the channel could be closed between reading the boolean and when we next call read(). In other words the bytes read could absolutely be -1 in that case and be valid.

question: Can CRT accept a 0 byte chunk with isFinalChunk set to true and do the right thing or does it require the last chunk and flag to come together?

We can't send a 0-byte chunk as the final chunk, it has to have some data. This is because CRT does a check on the size of the chunk and skips processing if it's 0.

I see the issue, I will refactor it

aajtodd · 2022-12-12T13:57:49Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+                    }
+                }
+                is HttpBody.ChannelContent -> {
+                    val chan = (request.body as HttpBody.ChannelContent).readFrom()


This cast seems like it should be caught by compiler smart casting and not be required explicitly, might be able to remove and just do request.body.readFrom()

I agree, but I tried it earlier and got this warning:

Smart cast to 'HttpBody.ChannelContent' is impossible, because 'request.body' is a public API property declared in different module

The compiler can't know this is safe because body is a val which, while always read-only, may be non-constant.

aajtodd · 2022-12-12T13:58:12Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+
+            when (request.body) {
+                is HttpBody.SourceContent -> {
+                    val source = (request.body as HttpBody.SourceContent).readFrom()


same comment about smart casting, might be worth checking if we can simplify to request.body.readFrom()

aajtodd · 2022-12-12T14:01:02Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+                    while (true) {
+                        source.read(buffer, bytesToRead)
+
+                        // read another set of bytes into a second buffer -- this is how we can "peek" to decide if this is the final chunk or not


fix: This double buffering logic is unnecessary.

Instead you can structure this like:

val source = request.body.readFrom() val bufferedSource = source.buffer() while(true) { bufferedSource.request(bytesToRead) bufferedSource.buffer.readByteArray() ... val isFinalChunk = bufferedSource.exhausted() ... }

aajtodd · 2022-12-12T14:02:31Z

...s/http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/RequestUtil.kt

            val source = body.readFrom()
            callContext.job.invokeOnCompletion {
                source.close()
            }
            SdkSourceBodyStream(source)
        }
+        else -> null


this should throw an error on the else clause now

…fers for ChannelContent

ianbotsf · 2022-12-12T19:10:17Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+                if (isFinalChunk) break else buffer = nextBuffer
+            }
+        }
+        else -> {}


Nit: else -> error("sendChunkedBody shouldn't be called for non-chunked body types") or something like that.

ianbotsf · 2022-12-12T19:12:18Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+            while (!chan.isClosedForRead) {
+                chan.read(buffer, CHUNK_BUFFER_SIZE)


Question: Is it possible on the initial iteration for isClosedForRead to return false but then the first read to return -1 as a result of suspension?

Yes its possible, although wouldn't that be an empty body? We wouldn't be able to handle that if I understood CRT requirements on sending empty chunks this morning.

Yeah it is possible. It's not necessarily an empty body, it could be a nonempty body that got closed prematurely for some reason. CRT wouldn't be able to handle it either way though. Should I try to handle this case and throw an exception or just let it blow up? It seems like a very unlikely thing to happen.

I advocate for additional handling of this case. When we have to debug a failure here, it'll be nice to have a clear indication what happened rather than a cryptic CRT exception.

Alright, I added some logic to handle this case where the initial chunk hasn't been sent but the channel is closed. Thanks!

aajtodd · 2022-12-12T19:20:17Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

+            while (!chan.isClosedForRead) {
+                chan.read(buffer, CHUNK_BUFFER_SIZE)


Yes its possible, although wouldn't that be an empty body? We wouldn't be able to handle that if I understood CRT requirements on sending empty chunks this morning.

aajtodd · 2022-12-12T19:22:02Z

...s/http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/RequestUtil.kt

+ * and the body is either [HttpBody.SourceContent] or [HttpBody.ChannelContent].
+ */
+internal val HttpRequest.isChunked: Boolean get() = (this.body is HttpBody.SourceContent || this.body is HttpBody.ChannelContent) &&
+    this.headers.getAll("Transfer-Encoding")?.contains("chunked") == true


nit: headers.contains("Transfer-Encoding", "chunked") already checks if any header contains the value

nit 2: You can generally leave off qualifying this on extension functions most of the time.

aajtodd · 2022-12-12T19:26:14Z

...http-client-engine-crt/common/src/aws/smithy/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt

@@ -131,3 +144,32 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE
        }
    }
 }
+
+internal suspend fun HttpStream.sendChunkedBody(body: HttpBody) {


nit: This extension doesn't feel like it belongs in this file, maybe move to RequestUtil.kt or something.

…written

sonarqubecloud · 2022-12-12T19:49:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

lauzadis added 10 commits December 9, 2022 13:16

add writeChunk override implementation

1a79215

bump CRT version

6afcee8

special-case aws-chunked requests

7a15e06

fix edge case and simplify SourceContent logic

e728eb3

simplify CRT request conversion logic

cde19cf

add changelog

69758ae

ktlint

88b606b

Add tests

8d07984

rename isAwsChunked to isChunked

93c12a6

ktlint

8306147

lauzadis marked this pull request as ready for review December 9, 2022 22:46

lauzadis requested a review from a team as a code owner December 9, 2022 22:46

ianbotsf reviewed Dec 9, 2022

View reviewed changes

clean up when block

f171fdc

aajtodd suggested changes Dec 12, 2022

View reviewed changes

lauzadis added 4 commits December 12, 2022 09:56

simplify SourceContent chunked upload

eff5c16

refactor chunked writes to HttpStream extension function. use two buf…

4a9cf27

…fers for ChannelContent

add tests

e4f9f63

ktlint

d33223d

lucix-aws approved these changes Dec 12, 2022

View reviewed changes

ianbotsf reviewed Dec 12, 2022

View reviewed changes

aajtodd approved these changes Dec 12, 2022

View reviewed changes

lauzadis added 2 commits December 12, 2022 13:32

move sendChunkedBody function. remove unnecessary usage of this

0df400f

handle case when channel is exhausted but initial chunk has not been …

54f70c8

…written

ianbotsf approved these changes Dec 12, 2022

View reviewed changes

lauzadis merged commit e1ea10e into main Dec 12, 2022

lauzadis mentioned this pull request Dec 12, 2022

chore: bump smithy-kotlin version awslabs/aws-sdk-kotlin#792

Merged

lauzadis mentioned this pull request Feb 2, 2023

Fix aws-chunked content encoding when using the CrtHttpEngine #759

Closed

lauzadis deleted the fix-aws-chunked-crt branch February 22, 2023 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rt): add special-casing for CrtHttpEngine `aws-chunked` requests #760

fix(rt): add special-casing for CrtHttpEngine `aws-chunked` requests #760

lauzadis commented Dec 9, 2022

ianbotsf Dec 9, 2022

lauzadis Dec 9, 2022

ianbotsf Dec 9, 2022

lauzadis Dec 9, 2022

ianbotsf Dec 9, 2022

lauzadis Dec 9, 2022

aajtodd Dec 12, 2022

lauzadis Dec 12, 2022

aajtodd Dec 12, 2022

aajtodd Dec 12, 2022

lauzadis Dec 12, 2022

aajtodd Dec 12, 2022

lauzadis Dec 12, 2022

aajtodd Dec 12, 2022

lauzadis Dec 12, 2022

ianbotsf Dec 12, 2022

aajtodd Dec 12, 2022

aajtodd Dec 12, 2022

aajtodd Dec 12, 2022

ianbotsf Dec 12, 2022

ianbotsf Dec 12, 2022

aajtodd Dec 12, 2022

lauzadis Dec 12, 2022

ianbotsf Dec 12, 2022

lauzadis Dec 12, 2022

aajtodd Dec 12, 2022

aajtodd Dec 12, 2022

aajtodd Dec 12, 2022

sonarqubecloud bot commented Dec 12, 2022

		while (!chan.isClosedForRead) {
		chan.read(buffer, CHUNK_BUFFER_SIZE)

fix(rt): add special-casing for CrtHttpEngine aws-chunked requests #760

fix(rt): add special-casing for CrtHttpEngine aws-chunked requests #760

Conversation

lauzadis commented Dec 9, 2022

Issue #

Description of changes

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

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

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

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 12, 2022

fix(rt): add special-casing for CrtHttpEngine `aws-chunked` requests #760

fix(rt): add special-casing for CrtHttpEngine `aws-chunked` requests #760