Skip to content

ResponseStream underlying buffer improvements #2351

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

Merged
merged 4 commits into from
Feb 16, 2023
Merged

ResponseStream underlying buffer improvements #2351

merged 4 commits into from
Feb 16, 2023

Conversation

SergeyRyabinin
Copy link
Contributor

@SergeyRyabinin SergeyRyabinin commented Feb 16, 2023

Issue #, if available:

#58, #1732, #2319

The issue:

  • ResponseStream design can attach to an underlying buffer it does not own, a double destruction may occur;
  • TransferManager marks a download as complete before the actual destination buffer is flushed.

Description of changes:

  • Use std::ios_base::register_callback to get notified if underlying stream is destructed;
  • Do not flush in ResponseStream d-tor: it does not make sense, on the next line, proper underlying stream implementation should do it anyway in it's destructor (and should not if flush is not required in their design).
  • Flush outcome's bodies in TransferManager before sending a complete notification;
  • Update Result's Get<Aws::IOStream object> to be const (for the change above);
    Check all that applies:
  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SergeyRyabinin SergeyRyabinin force-pushed the sr/flush branch 2 times, most recently from c310e53 to 47da845 Compare February 16, 2023 19:49
@SergeyRyabinin SergeyRyabinin marked this pull request as ready for review February 16, 2023 22:43
{
if (!m_underlyingStream)
{
assert(m_underlyingStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log first and fail assert after during debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are equal in messaging to the developer.
Except FATAL tracing is just a last resort if library is built in Release mode.
It is a really just "not crash", the next line will highly likely produce a memory leak reported by custom allocator set in the API Handle.

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.

3 participants