Skip to content
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

fix: improve detection of available read bytes to avoid hang #535

Merged
merged 4 commits into from
Feb 23, 2022

Conversation

ianbotsf
Copy link
Contributor

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.

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang

@ianbotsf ianbotsf marked this pull request as ready for review February 23, 2022 00:32
@ianbotsf ianbotsf requested a review from a team as a code owner February 23, 2022 00:32
@ianbotsf ianbotsf requested a review from aajtodd February 23, 2022 00:32
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple suggestions but otherwise looks good.

cont.resume(true)
} else {
val success = readOp.compareAndSet(null, cont)
check(success) { "Read operation already in progress" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" }
}

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-fix-crt-stream-hang

@ianbotsf ianbotsf merged commit 4bf1167 into main Feb 23, 2022
@ianbotsf ianbotsf deleted the fix-crt-stream-hang branch February 23, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetObject api with default http client hangs while downloading large file
2 participants