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

ByteStreamJVM.writeToOutputStream is documented as not closing the provided stream, but does close it #1444

Closed
1 task done
cloudshiftchris opened this issue Oct 17, 2024 · 4 comments · Fixed by smithy-lang/smithy-kotlin#1172
Assignees
Labels
bug This issue is a bug. p1 This is a high priority issue

Comments

@cloudshiftchris
Copy link

cloudshiftchris commented Oct 17, 2024

Describe the bug

ByteStreamJVM.writeToOutputStream is documented as This method does not flush or close the given OutputStream. but does close the stream (indirectly).

outputStream.sink() ultimately creates an okio.OutputStreamSink that has override fun close() = out.close()

We ran across this writing S3 objects (in a loop) to a ZipOutputStream that was closed after the first call to body.writeOutputStream(...).

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected behavior

Expecting that writing to a stream does not close it. One may wish to write further data to the stream, as is the case with a ZipOutputStream with multiple entries.

Current behavior

writeOutputStream closes the stream, contradicting the documented behaviour.

Steps to Reproduce

Pass a stream into any ByteStream.writeToOutputStream and check its state after the call.

Possible Solution

Wrap in some form of close-suppressing sink, or a close-suppressing OutputStream.

We worked around this by doing body.toInputStream().copyTo(outputStream) (which isn't great, that InputStream may or may not need to be closed...)

Context

Good for now - pls restore the documented functionality!

AWS SDK for Kotlin version

1.3.52 (unclear where the regression happened, or if this ever worked as documented)

Platform (JVM/JS/Native)

JVM

Operating system and version

any

@cloudshiftchris cloudshiftchris added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 17, 2024
@github-actions github-actions bot added the potential-regression Marking this issue as a potential regression to be checked by team member label Oct 17, 2024
@0marperez 0marperez added p1 This is a high priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Oct 18, 2024
@0marperez
Copy link
Contributor

0marperez commented Oct 18, 2024

Hi, thanks for the report. I've set this as high priority and will begin working on it as soon I'm finished with unrelated work as part of my on call shift.

@0marperez 0marperez added p1 This is a high priority issue and removed p1 This is a high priority issue potential-regression Marking this issue as a potential regression to be checked by team member labels Oct 18, 2024
@lauzadis lauzadis self-assigned this Oct 30, 2024
@lauzadis
Copy link
Member

Hello and thanks for the report, I was able to replicate the issue and am working on a fix.

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.

@lauzadis
Copy link
Member

Hello, we've merged a fix for this issue and it'll be included in an upcoming release. writeToOutputStream will maintain its existing behavior to ensure backwards compatibility, and a new appendToOutputStream method was added which does not close the stream.

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. p1 This is a high priority issue
Projects
None yet
3 participants