Chunking NG: SHA1 checksumming of whole file #11811 #26582
Conversation
|
@guruz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @icewind1991 to be potential reviewers. |
| if ($checksum && isset($_SERVER['HTTP_OC_CHECKSUM'])) { | ||
| if (strpos($_SERVER['HTTP_OC_CHECKSUM'], $checksum) === false) { | ||
| // We use "strpos" because client might send multiple checksums | ||
| throw new BadRequest('invalid checksum computed ' . $checksum . ' got ' . $_SERVER['HTTP_OC_CHECKSUM']); |
There was a problem hiding this comment.
From a users PoV:
Please make sure that this message has some more context as the well known "expected filesize xxx, got yyy" message was just confusing and people didn't know what to do with that.
There was a problem hiding this comment.
Do you have a proposal? :) "Corrupted file stream"? "Corrupted upload"?
There was a problem hiding this comment.
Unfortunately not as i don't know what is causing that message. If it will the caused by a Corrupted upload i would name it like this.
There was a problem hiding this comment.
"Corrupted upload, likely related to network issues. Please try again" ? 😉
There was a problem hiding this comment.
If thats the explanation for the reason i would go for this or a similar text. This will safe us/you from user questions about the actual meaning of the current text.
|
Code looks good, as discussed. Great work! But also as discussed, I'm a bit worried that it might decrease the performance a bit like observed in the past #11811 (comment) (that was with md5 though). We could add a switch to disable checksumming altogether. Or at least disable the calculation if the client did not send any checksum (client could be any other Webdav client). @DeepDiver1975 @butonic @PhilippSchaffrath thoughts ? |
Not in this case because chunking ng is only used by the oC client (and hopefully the webinterface at some point) (The table in the linked comment is from 2y ago, now might also be much better) |
|
@guruz hmm indeed. And now I realize that we actually have yet another mechanism to upload file to the new endpoint: simple PUT. Will the client still use chunking ng for small files with the benefit of checksums ? |
|
See my comment on the approach: #26655 (comment) |
|
@mrow4a can I ask you to do a performance comparison? What is the overhead for this? THX |
|
I belive you are mostly interested in worst case scenario? So what will be the overhead using standard VM with standard HDD and 2-4 CPUs? Will do that, I am actualy also currious. |
|
I did some tests. I have been running that on test machine with : I couldnt belive actualy the results, but since you do it in memory.. :> I rerun the test for 1x100MB files, so 10 chunks, 20 times: I did not belive that and I actualy even started printing checksums.. But it is like that. Seems in-memory checksums are nearly for free, due to pipelining of requests the difference is nearly unspottable. The way you do it also probably will not show anything revolutionary in blackfire, since you do it in-memory anyway. |
|
I would prefer to calculate the hash by wrapping the php://input stream. typically network is even slower than disk access. I tried to grasp when the AssemblyStream is used ... but it is far to late for me now. n8 Oh that does not mean you should change this right now. This is awesome! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is for owncloud/client#5102 (comment)