fix: ensure CRT doesn't starve the dispatcher #294
Conversation
ianbotsf
left a comment
ianbotsf
left a comment
There was a problem hiding this comment.
Minor issue otherwise looks good.
| testLogging { | ||
| events("passed", "skipped", "failed") | ||
| showStandardStreams = true | ||
| } |
There was a problem hiding this comment.
Fix: Generally we shouldn't show full output on passed tests. Was this left in here for debug?
There was a problem hiding this comment.
It's the same settings we use everywhere else:
- https://github.com/awslabs/aws-sdk-kotlin/blob/065e6fbe87b1cbbe52a3376f9b9f14fc80ae7d3c/services/build.gradle.kts#L44-L50
- https://github.com/awslabs/aws-sdk-kotlin/blob/065e6fbe87b1cbbe52a3376f9b9f14fc80ae7d3c/gradle/jvm.gradle#L28-L32
I'm in favor of leaving it since most of our tests aren't printing anything anyway and it allows easier debugging if you do without changing build settings.
There was a problem hiding this comment.
It appears there is precedent. Very well, my concern for this PR is addressed.
| // see: https://github.com/awslabs/aws-sdk-kotlin/issues/282 | ||
| // | ||
| // TODO - we could get rid of this extra copy + coroutine if readAvailable() had a non-suspend version | ||
| // see: https://youtrack.jetbrains.com/issue/KTOR-2772 |
There was a problem hiding this comment.
side question
doesn't look like this ticket is getting much love. perhaps something to bring up in our next meeting?
There was a problem hiding this comment.
perhaps, we could also probably go submit a PR if we felt the need
3 participants
Issue #
fixes #282
Description of changes
Provide a means to allow
ByteStream.Streaminginputs to be consumed by the CRT. The ticket has more details on the issue but basically the proxy coroutine we were using previously would launch, fill as much as could be read, then suspend waiting for more data. The CRT would start reading and then basically starve the dispatcher such that the proxy coroutine was never able to make progress (and since the proxy coroutine is the one that could unblock the CRT to make progress it results in an infinite loop).This solution is not efficient but it does work. Instead we launch a coroutine on demand when we know there is data available to read. The coroutine is launched undispatched which means it will run to the first suspension point (which we won't hit because we know there is data available).
The previously failing e2eTest now passes.
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.