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.

Sign up for GitHub

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

Jump to bottom

Fix empty file uploads to S3 (and other streaming storages) #14210

Merged
merged 4 commits into from
Feb 18, 2019
Merged

Fix empty file uploads to S3 (and other streaming storages) #14210

merged 4 commits into from
Feb 18, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Feb 14, 2019 •
edited by MorrisJobke
Loading

Fixes #14192

TODO:

@rullzer rullzer added this to the Nextcloud 16 milestone Feb 14, 2019
@rullzer
Fix empty file uploads to S3
61d66d7 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Add a wrapper to determine if a file is EOF
3720add 
The stream is already closed at this point. Which means feof will always
return false. We have to determine if the stream is EOF in the
preCloseCallback. And pass this info along. Then the logic works as
expected.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the fix/14192/fix_empty_uploads branch from da4e23c to 2e622fd Compare February 15, 2019 07:42
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 15, 2019
@rullzer rullzer force-pushed the fix/14192/fix_empty_uploads branch from 2e622fd to 6e4c98f Compare February 15, 2019 09:30
@rullzer
Copy link
Member Author

rullzer commented Feb 15, 2019

Rebased

@MorrisJobke
Copy link
Member

before: 💥
after: upload works

There are still some warnings in the log:

{"reqId":"9bcr1G2Wjy9bPCjeibs4","level":3,"time":"2019-02-15T10:49:57+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"PHP","method":"PUT","url":"\/remote.php\/webdav\/a","message":"A non-numeric value encountered at \/Users\/morris\/Projects\/nextcloud\/server\/lib\/private\/Files\/ObjectStore\/S3ObjectTrait.php#81","userAgent":"Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_14_2) AppleWebKit\/605.1.15 (KHTML, like Gecko) Version\/12.0.2 Safari\/605.1.15","version":"16.0.0.0"}
{"reqId":"6Mutt2YZH11krtCeaH6R","level":3,"time":"2019-02-15T10:49:58+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"PHP","method":"GET","url":"\/index.php\/apps\/theming\/img\/core\/filetypes\/file.svg?v=0","message":"A non-numeric value encountered at \/Users\/morris\/Projects\/nextcloud\/server\/lib\/private\/Files\/ObjectStore\/S3ObjectTrait.php#81","userAgent":"Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_14_2) AppleWebKit\/605.1.15 (KHTML, like Gecko) Version\/12.0.2 Safari\/605.1.15","version":"16.0.0.0"}
{"reqId":"6Mutt2YZH11krtCeaH6R","level":3,"time":"2019-02-15T10:49:58+00:00","remoteAddr":"127.0.0.1","user":"admin","app":"PHP","method":"GET","url":"\/index.php\/apps\/theming\/img\/core\/filetypes\/file.svg?v=0","message":"A non-numeric value encountered at \/Users\/morris\/Projects\/nextcloud\/server\/lib\/private\/Files\/ObjectStore\/S3ObjectTrait.php#81","userAgent":"Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_14_2) AppleWebKit\/605.1.15 (KHTML, like Gecko) Version\/12.0.2 Safari\/605.1.15","version":"16.0.0.0"}

@rullzer
Copy link
Member Author

rullzer commented Feb 15, 2019

Needs icewind1991/Streams#10

@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 15, 2019
@rullzer rullzer mentioned this pull request Feb 15, 2019
Merged
@rullzer rullzer force-pushed the fix/14192/fix_empty_uploads branch from 6e4c98f to 6ec3173 Compare February 15, 2019 14:50
@rullzer
Copy link
Member Author

rullzer commented Feb 15, 2019

@MorrisJobke fixed with nextcloud/3rdparty#242 please have another look 😄

MorrisJobke
MorrisJobke approved these changes Feb 15, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke
Copy link
Member

It's merged now 👍

@rullzer
Bump icewind/streams for supported streams
3edffdd 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the fix/14192/fix_empty_uploads branch from 6ec3173 to 3edffdd Compare February 18, 2019 12:04
@rullzer
Copy link
Member Author

rullzer commented Feb 18, 2019

Rebased. Then lets get this in.

@rullzer
Make sure that we have a stream
fc967a5 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the fix/14192/fix_empty_uploads branch from 0deae8b to fc967a5 Compare February 18, 2019 14:00
@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Feb 18, 2019
@rullzer rullzer merged commit f6f002e into master Feb 18, 2019
@rullzer rullzer deleted the fix/14192/fix_empty_uploads branch February 18, 2019 14:39
@rullzer rullzer mentioned this pull request Feb 18, 2019
Merged
2 tasks
@kesselb kesselb mentioned this pull request Aug 11, 2019
Closed
@juliusknorr juliusknorr mentioned this pull request Dec 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icewind1991 icewind1991 icewind1991 approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug regression
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.
3 participants
@rullzer @MorrisJobke @icewind1991