Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jan 25, 2019

Fixes #13062

S3 requires seekable streams to calculate the SHA256 checksum. Since the
assembly stream (from the chunking) isn't seekable we have to find a way
around this.

For now this just disables the use of writing a stream directly until we
have a better fix.

Signed-off-by: Roeland Jago Douma [email protected]

Fixes #13062

S3 requires seekable streams to calculate the SHA256 checksum. Since the
assembly stream (from the chunking) isn't seekable we have to find a way
around this.

For now this just disables the use of writing a stream directly until we
have a better fix.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member Author

rullzer commented Jan 25, 2019

@rullzer
Copy link
Member Author

rullzer commented Jan 25, 2019

/backport to stable15

}

if ($partStorage->instanceOfStorage(Storage\IWriteStreamStorage::class)) {
if (false && $partStorage->instanceOfStorage(Storage\IWriteStreamStorage::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't instead S3 not implement IWriteStreamStorage anymore?
Other storages might support it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also possible yes. I just did the quick fix that made sure that other backends with possible bugs also are not affected. But yes maybe that is cleaner.
Let me fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah not so easy. As the S3 implementation extends Common. So I propose just to get this in and do more extensive fixes later.

@icewind1991
Copy link
Member

From looking trough it's code, I think that the MultipartUploader does handle non seekable streams correctly.

Additionally we can make the assembly stream seekable

@MorrisJobke
Copy link
Member

@icewind1991 So your implementation in #13866 is superseding this one here?

@icewind1991
Copy link
Member

yes

@kesselb
Copy link
Contributor

kesselb commented Feb 7, 2019

Looks like some issues with webdav are solved by this "fix" as well. Please have a look 😎

#13639 #13724 #14079

@IngoEF
Copy link

IngoEF commented Feb 7, 2019

sorry asking this: is it operationally safe now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upload is broken with S3 object storage

7 participants