Skip to content

TransferManager::DoSinglePartDownload() should flush the stream before marking the transfer completed #1732

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

Closed
igorcanadi opened this issue Aug 10, 2021 · 4 comments
Assignees
Labels
bug This issue is a bug. needs-reproduction This issue needs reproduction. p2 This is a standard priority issue

Comments

@igorcanadi
Copy link

TransferManager::DoSinglePartDownload() marks the transfer as completed here: https://github.com/rockset/rocksdb-cloud/blob/8c0ff476d77c0df0af92b1466269404bdfb8e89e/cloud/aws/aws_s3.cc#L196

This happens before the underlying response stream is flushed. The response stream's ownership is tied to local variable getObjectOutcome and the stream is flushed only in getObjectOutcome's destructor when it out of scope (here:

).

This means that a code that uses std::fstream to download S3 object to a local file and does:

handle->WaitUntilFinished();
// do something with the file

is incorrect, because the file might not be flushed when WaitUntilFinished() returns.

This is similar to this 6 year old bug: #58

@igorcanadi igorcanadi added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 10, 2021
@KaibaLopez
Copy link
Contributor

Hi @igorcanadi ,
Yea thanks for bringing attention to this,and you're right, this is an ongoing issue with the SDK, I'll try to make it a priority for the team.

@KaibaLopez KaibaLopez added needs-review This issue or pull request needs review from a core team member. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 13, 2021
@igorcanadi
Copy link
Author

Hi @KaibaLopez any news on this issue? Thanks!

@jmklix jmklix added needs-reproduction This issue needs reproduction. p2 This is a standard priority issue and removed needs-review This issue or pull request needs review from a core team member. labels Dec 16, 2022
@jmklix jmklix self-assigned this Dec 16, 2022
@SergeyRyabinin
Copy link
Contributor

Hi,
I think it should be fixed now by #2351
At least a flush() is called before a task completion notification.
Closing the issue, please submit a new one or re-open should you see more issues.

Best regards,
Sergey

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-reproduction This issue needs reproduction. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

4 participants