fix: handle stream response exceptions properly in Ktor engine #589
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
I don't recall the behavior of kotlinx.coroutines.Mutex w.r.t same thread. This refactor could be susceptible to incorrect behavior if the mutex is re-entrant (i.e. same thread that locked it is allowed to acquire).
I don't think that the mutex is re-entrant (especially since there is an explicit re-entrant lock type in kotlinx.atomicfu package) but worth maybe double checking.
| delay(500.milliseconds) | ||
| waiter.signal() | ||
| } | ||
| delay(1000.milliseconds) |
There was a problem hiding this comment.
we might consider changing these to something smaller, no need to wait a full second in the test I don't think
There was a problem hiding this comment.
This is in a runBlockingTest so time has no meaning.
According to the |
Issue #
aws-sdk-kotlin#524
Description of changes
Fixes a bug with the Ktor engine's waiter. Due to a bad refactor, the implementation is unable to handle exceptions that occur before the waiter begins waiting (e.g., in the case an exception is thrown). Rather than restore the previous implementation based on channels, switched for a simpler and (hopefully) clearer implementation based on mutexes.
Added unit tests that verify waiter behavior. In particular,
testSignalWhenNotWaitingfailed with the prior implementation.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.