-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dokka and coroutine version bump #514
Conversation
… refactoring needs
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good (minus the new @Ignore
tests obvs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, feel free to reach out for help on the CRT issues if needed.
I guess I should have commented in smithy-kotlin
PR but I was surprised the "ManualDispatch" tests (BufferedReadChannelTest.kt
and BufferReadChannelByteBufferTest.kt
) Just Work (TM) out of the box. Before the runTest
implementation in the base class would call pauseDispatcher()
, do you understand why these work out of the box with the new runTest
implementation?
@@ -40,7 +42,8 @@ class AsyncStressTest : TestWithLocalServer() { | |||
} | |||
|
|||
@Test | |||
fun testConcurrentRequests() = runSuspendTest { | |||
@Ignore // FIXME: Test fails after kotlinx-coroutines-test 1.6.0 upgrade | |||
fun testConcurrentRequests() = runTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these fail with runTest
I would just make it runBlocking
and see if it continues to work. Can you expand on the failures that are happening though? This isn't necessarily one that I thought would require attention (although anything with CRT can be fragile due to the threading model and how it interacts with coroutines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible that we are reliant on a particular dispatcher behavior in which case we might need to look closer at the internals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a snippet from the error when enabling the test. Happily runBlocking
seems to fix it up!
[Test worker @coroutine#2] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server listening on: localhost:46491
exception on 460: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool
exception on 454: java.util.concurrent.CancellationException: Parent job is Cancelling
...
exception on 461: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool
exception on 462: aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool
...
[Test worker] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server stopped
timed out waiting for an HTTP connection to be acquired from the pool
aws.sdk.kotlin.runtime.ClientException: timed out waiting for an HTTP connection to be acquired from the pool
at app//aws.sdk.kotlin.runtime.http.engine.crt.CrtHttpEngine.roundTrip(CrtHttpEngine.kt:81)
@Test | ||
fun testPutObjectFromFile() = runSuspendTest { | ||
@Ignore // FIXME: CRT HTTP client fails to get connection after kotlinx-coroutines-test 1.6.0 upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these CRT issues lead me to believe it's probably the dispatcher, my guess is some interaction between the dispatcher and CRT trying to read the request body (or send the response body).
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
1 similar comment
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
@@ -73,9 +72,9 @@ class AsyncStressTest : TestWithLocalServer() { | |||
} | |||
} | |||
|
|||
@IgnoreWindows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Can we please add a reason to this annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the test run in which the regression occurs on Windows: https://github.com/awslabs/aws-sdk-kotlin/runs/5057351434?check_suite_focus=true
Here is the error snippet from the log:
AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_ERROR
[DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Autoreload is disabled because the development mode is off.
[DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Responding at http://0.0.0.0:51019/
[DefaultDispatcher-worker-1 @coroutine#2078] INFO ktor.application - Application started in 0.013 seconds.
[Test worker @coroutine#2079] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server listening on: localhost:51019
AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_OUT
exception on 973: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 974: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 975: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 976: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 977: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 982: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 981: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 985: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 987: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 978: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 979: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 983: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 986: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 980: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 984: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 988: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 989: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 990: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 991: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 992: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 993: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 994: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 995: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 996: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 997: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 998: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
exception on 999: java.util.concurrent.CancellationException: Timed out waiting for 5000 ms
AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] STANDARD_ERROR
[Test worker] INFO aws.smithy.kotlin.runtime.httptest.TestWithLocalServer - test server stopped
AsyncStressTest[jvm] > testStreamNotConsumed()[jvm] FAILED
kotlinx.coroutines.TimeoutCancellationException at �:-1
Caused by: kotlinx.coroutines.TimeoutCancellationException at Timeout.kt:184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK IDK if we can merge this without looking into this further to ensure we aren't introducing a regression on windows (i.e. is this an issue with the test surfaced by the upgrade OR a latent issue with the implementation discovered in this test by the upgrade)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -67,6 +67,9 @@ subprojects { | |||
kotlinOptions { | |||
jvmTarget = "1.8" // this is the default but it's better to be explicit (e.g. it may change in Kotlin 1.5) | |||
allWarningsAsErrors = false // FIXME Tons of errors occur in generated code | |||
// Enable coroutine runTests in 1.6.10 | |||
// NOTE: may be removed after coroutines-test runTests becomes stable | |||
freeCompilerArgs = freeCompilerArgs + "-opt-in=kotlin.RequiresOptIn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it that way originally but the IDE warned me of the creation of a list under the hood and recommended this form instead.
kotlinOptions { | ||
// Enable coroutine runTests in 1.6.10 | ||
// NOTE: may be removed after coroutines-test runTests becomes stable | ||
freeCompilerArgs = freeCompilerArgs + "-opt-in=kotlin.RequiresOptIn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it that way originally but the IDE warned me of the creation of a list under the hood and recommended this form instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the ignored test for windows looks good.
@@ -73,9 +72,9 @@ class AsyncStressTest : TestWithLocalServer() { | |||
} | |||
} | |||
|
|||
@IgnoreWindows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this now?
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
A new generated diff is ready to view: __generated-main...__generated-chore-coroutine-version-bump |
Companion PR: smithy-lang/smithy-kotlin#580
Issue #
#513
Description of changes
Duration
type as of Kotlin 1.6NOTE
9 tests2 tests has been marked with@IgnoreWindows
due to test failure and regression issues associated with thecoroutine-test
update.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.