fix: improve detection of available read bytes to avoid hang #535
Conversation
|
A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang |
|
A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang |
|
A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang |
aajtodd
left a comment
aajtodd
left a comment
There was a problem hiding this comment.
Couple suggestions but otherwise looks good.
| cont.resume(true) | ||
| } else { | ||
| val success = readOp.compareAndSet(null, cont) | ||
| check(success) { "Read operation already in progress" } |
There was a problem hiding this comment.
this could cause mutex to be in a bad state. It's not really a recoverable error since it would mean multiple readers but we may want to consider restructuring the code anyway to ensure that all paths unlock the mutex. In particular because the CRT threads will be hosed by this.
Also we already check availableForRead > 0 while holding the lock on L75 so we don't really need to check again inside suspendCancellableCoroutine since that state can't change at this point.
e.g.
return suspendCancellableCoroutine { cont ->
val success = readOp.compareAndSet(null, cont)
readOpMutex.unlock()
check(success) { "Read operation already in progress" }
}
|
Kudos, SonarCloud Quality Gate passed!
|
|
A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang |
2 participants
Issue #
Closes #525
Description of changes
This addresses a potential hang that occurs in CRT streaming code when the read continuation is set after a series of writes which consumed the entire window capacity. The fix here is a double-check before setting the continuation so that the code can avoid suspending if possible.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.