feat(rt): implement interceptors #769

aajtodd · 2022-12-16T20:29:16Z

Issue #

#122

Description of changes

feat: Implements the runtime design for interceptors
refactor: Make HttpRequest an interface and allow HttpRequestBuilder to implement it. This allows for the COW (copy-on-write) semantics we want for interceptor modify hooks but is cheap for all the read hooks (and if the modify hooks do nothing)
- This is somewhat fallout from the fact that HttpRequestBuilder is what flow through our operation middleware. We don't make it immutable until we are ready to send it to the engine usually.

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

…ng a builder as an immutable view of HttpRequest

…ptors honest

ianbotsf · 2022-12-19T18:12:08Z

runtime/protocol/http-test/common/src/aws/smithy/kotlin/runtime/httptest/RecordingConnection.kt

-        val requestCopy = request.copy(body = reqBody)
+        val requestCopy = HttpRequest(method = request.method, url = request.url, headers = request.headers, body = reqBody)


Question: Is it worth supporting a copy method on HttpRequest or its implementations? Or would this be better written as request.toBuilder().build()?

We could add one, we only had one or two usages of it though so I opted to just do it manually.

ianbotsf · 2022-12-19T18:18:35Z

.../protocol/http/common/src/aws/smithy/kotlin/runtime/http/interceptors/InterceptorExecutor.kt

+    // This changes as execution progresses, it represents the most up-to-date version of the operation input.
+    // If we begin executing an operation at all it is guaranteed to exist because `readBeforeExecution` is the first
+    // thing invoked when executing a request
+    private var _lastInput: I? = null
+
+    // Track most up to date http request and response. The final two hooks do not have easy access to this data
+    // so we store it as execution progresses
+    private var _lastHttpRequest: HttpRequest? = null
+    private var _lastHttpResponse: HttpResponse? = null


Question: Why the underscore prefixes on these variables?

ianbotsf · 2022-12-19T18:19:18Z

.../protocol/http/common/src/aws/smithy/kotlin/runtime/http/interceptors/InterceptorExecutor.kt

+    // This changes as execution progresses, it represents the most up-to-date version of the operation input.
+    // If we begin executing an operation at all it is guaranteed to exist because `readBeforeExecution` is the first
+    // thing invoked when executing a request
+    private var _lastInput: I? = null
+
+    // Track most up to date http request and response. The final two hooks do not have easy access to this data
+    // so we store it as execution progresses
+    private var _lastHttpRequest: HttpRequest? = null
+    private var _lastHttpResponse: HttpResponse? = null


Question: Is multithreaded access a concern here? Should these be wrapped in atomics?

InterceptorExecutor is scoped to a single operation. All the hooks should fire from one thread (may not be the same thread for the entirety of the operation because coroutines can switch after suspension points but only one thread should ever be interacting with it).

Is there a scenario where you see this as not holding true?

No, that makes sense.

ianbotsf · 2022-12-19T18:20:10Z

.../protocol/http/common/src/aws/smithy/kotlin/runtime/http/interceptors/InterceptorExecutor.kt

+    private fun <T> checkType(phase: String, expected: KClass<*>, actual: Any): T {
+        check(expected.isInstance(actual)) { "$phase invalid type conversion: found ${actual::class}; expected $expected" }
+        @Suppress("UNCHECKED_CAST")
+        return actual as T
+    }
+    private fun <T> checkResultType(phase: String, result: Result<Any>, expected: KClass<*>): Result<T> =
+        result.map { checkType(phase, expected, it) }


Style: Missing empty line between multi-line member definitions.

Stupid ktlint!

ianbotsf · 2022-12-19T18:41:11Z

.../protocol/http/common/src/aws/smithy/kotlin/runtime/http/interceptors/InterceptorExecutor.kt

+    private fun <T> checkType(phase: String, expected: KClass<*>, actual: Any): T {
+        check(expected.isInstance(actual)) { "$phase invalid type conversion: found ${actual::class}; expected $expected" }
+        @Suppress("UNCHECKED_CAST")
+        return actual as T
+    }


Suggestion: You can use contracts to indicate that a non-exceptional return implies narrowed type information for actual. This would help avoid recasts in the calling method:

@ExperimentalContracts private inline fun <reified T> checkType(phase: String, expected: KClass<*>, actual: Any): T { contract { returns() implies (actual is T) } check(expected.isInstance(actual)) { "$phase invalid type conversion: found ${actual::class}; expected $expected" } @Suppress("UNCHECKED_CAST") return actual as T }

Oddly enough I had this at one point. We can add it back but in most cases it doesn't improve the calling method due to how they are structured (e.g. compiler is smart enough inside of fold{} blocks but outside of them it sees Any again). I removed it when I had to revisit how type checking was done, I can add it back.

Having issues getting this to work. I'm going to leave it for now.

ianbotsf · 2022-12-19T19:49:20Z

...e/protocol/http/common/src/aws/smithy/kotlin/runtime/http/operation/SdkOperationExecution.kt

+    override suspend fun call(request: OperationRequest<Input>): Output {
+        val result = interceptors.readBeforeExecution(request.subject)
+            .mapCatching {
+                inner.call(request)
+            }
+            .let {
+                interceptors.modifyBeforeCompletion(it)
+            }
+
+        interceptors.readAfterExecution(result)
+
+        return result.getOrThrow()
+    }


Question: I looks like a failure in readAfterExecution will override any failures in previous interceptors. Should they be added as suppressed exceptions?

Yeah I wasn't sure. I can see it either way. Do you have a strong opinion one way or another here?

ianbotsf · 2022-12-19T21:15:48Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt

+internal data class ImmutableHttpRequestBuilder(
+    internal val builder: HttpRequestBuilder,
+    internal val allowToBuilder: Boolean,
+) : HttpRequest {


Nit: This class doesn't feel like a "builder". It's not externally mutable and may be turned into a builder by the toBuilder() extension method. Maybe it's an ImmutableHttpRequest or an HttpRequestBuilderView?

ianbotsf · 2022-12-19T21:18:17Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt

+    private var _url: Url? = null
+    override val url: Url
+        get() = _url ?: builder.url.build().also { _url = it }
+
+    override val headers: Headers
+        get() = builder.headers.build()


Question: Is there a reason to memoize url but not headers?

ianbotsf · 2022-12-19T21:28:30Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/retries/Outcome.kt

+/**
+ * Convert an outcome to a [Result]
+ */
+@InternalApi
+public fun <T> Outcome<T>.toResult(): Result<T> = when (this) {


Comment: While we created this specifically for retry middleware, it seems generally useful and I can't think of a reason it shouldn't be public (non-@InternalApi).

ianbotsf · 2022-12-19T21:31:06Z

runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt

+    private var _url: Url? = null
+    override val url: Url
+        get() = _url ?: builder.url.build().also { _url = it }


Nit:

override val url: Url by lazy { builder.url.build() }

sonarqubecloud · 2022-12-21T02:08:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
20 Code Smells

No Coverage information
1.7% Duplication

aajtodd added 16 commits December 15, 2022 16:36

wip interceptor design

1d01e47

move interfaces around, add docs

8bfeae8

cleanup nullability of phases

d44437b

remove mutable generics

c915aec

refactor(rt)!: convert HttpRequest into an interface and allow treati…

7ca367c

…ng a builder as an immutable view of HttpRequest

poc wireup

c8666aa

wire up more hooks

ba82f10

add test for interceptor order

12f98f1

add type conversion tests

c160dd2

wire up retry hooks

be220c0

cleanup tests

494f2dd

add sanity test and refactor test utils

56d1d4b

more tests

17dbd91

update docs and fix last hooks

5c13cad

move internals around

e5c4543

triage fixme and todo; add flag controlling tobuilder to keep interce…

236839c

…ptors honest

aajtodd marked this pull request as ready for review December 19, 2022 13:36

aajtodd requested a review from a team as a code owner December 19, 2022 13:36

ianbotsf approved these changes Dec 19, 2022

View reviewed changes

lucix-aws approved these changes Dec 19, 2022

View reviewed changes

aajtodd added 2 commits December 20, 2022 14:45

cleanup

0ac9f95

add original result failure as suppressed exception

eb8f73e

lauzadis approved these changes Dec 21, 2022

View reviewed changes

aajtodd merged commit d8aa7b5 into feat-interceptors Dec 21, 2022

aajtodd deleted the impl-interceptors branch December 21, 2022 20:54

aajtodd added a commit that referenced this pull request Jan 6, 2023

feat(rt): implement interceptors (#769)

ec5ac1f

aajtodd mentioned this pull request Jan 6, 2023

feat: interceptors #775

Merged

aajtodd pushed a commit that referenced this pull request Mar 11, 2024

chore: improve build properties for service clients (#769)

eb5640e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rt): implement interceptors #769

feat(rt): implement interceptors #769

aajtodd commented Dec 16, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 20, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 20, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 19, 2022

ianbotsf Dec 19, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 19, 2022

aajtodd Dec 19, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 19, 2022

aajtodd Dec 20, 2022

ianbotsf Dec 19, 2022

aajtodd Dec 20, 2022

ianbotsf Dec 19, 2022

ianbotsf Dec 19, 2022

ianbotsf Dec 19, 2022

ianbotsf Dec 19, 2022

sonarqubecloud bot commented Dec 21, 2022

		val requestCopy = request.copy(body = reqBody)
		val requestCopy = HttpRequest(method = request.method, url = request.url, headers = request.headers, body = reqBody)

feat(rt): implement interceptors #769

feat(rt): implement interceptors #769

Conversation

aajtodd commented Dec 16, 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

sonarqubecloud bot commented Dec 21, 2022