Handle return code of streamCopy in WebDAV put #20927
Conversation
|
By analyzing the blame information on this pull request, we identified @icewind1991, @PVince81 and @DeepDiver1975 to be potential reviewers |
|
Looks good 👍 |
|
👍 |
| if (isset($_SERVER['CONTENT_LENGTH'])) { | ||
| $expected = $_SERVER['CONTENT_LENGTH']; | ||
| } | ||
| throw new BadRequest('error while copying file to target location (copied bytes: ' . $count . ', expected filesize: '. $expected .' )'); |
There was a problem hiding this comment.
Is this really a bad request?
There was a problem hiding this comment.
good catch, sounds rather like an internal server error.
There was a problem hiding this comment.
Therefore I added the discussion label. What is the wanted exception for the Sabre stack? cc @DeepDiver1975 @PVince81 ?
There was a problem hiding this comment.
I looked at the HTTP codes but didn't find anything adequate.
The only one maybe, if it was about external storages, would be "408 Request Timeout".
There is also "422 Unprocessable Entity" when the data uploaded did not match expectations, but that's more of a validation issue.
There was a problem hiding this comment.
From what I read "Bad request" usually tells the client to NOT repeat the request.
So it's more like "500 Internal Server error" then... Just throw a regular \Sabre\Exception, it will be 500.
|
There is one valid case where Other failures could be when writing to external storage but the connection took too long to respond, or the network connection to the ext storage is flaky. |
MorrisJobke
force-pushed
the
handle-return-code-on-webdav-put
branch
from
58eae5e to
3dc3448
Compare
| if (isset($_SERVER['CONTENT_LENGTH'])) { | ||
| $expected = $_SERVER['CONTENT_LENGTH']; | ||
| } | ||
| throw new Exception('Error while copying file to target location (copied bytes: ' . $count . ', expected filesize: '. $expected .' )'); |
There was a problem hiding this comment.
Please use a non-cryptic exception text. Most non-tech savy people having a look at their logfiles probably have no idea what this means and knows what to do when getting this exception.
There was a problem hiding this comment.
@PVince81 @icewind1991 @dragotin Do you have a better exception text in mind? I would change that to something more meaningful happily
There was a problem hiding this comment.
I'm fine with this. At least it makes it possible to distinguish the error from the other one.
* throw a different exception if streamCopy failed
PVince81
force-pushed
the
handle-return-code-on-webdav-put
branch
from
3dc3448 to
43f5d8a
Compare
|
Rebased to wake up CI |
|
👍 |
Handle return code of streamCopy in WebDAV put
|
@karlitschek @MorrisJobke backport to 8.2, 8.1, 8.0 ? |
Fine from my side |
|
yes. makes a ton of sense. please backport 👍 |
|
@MorrisJobke agreed. |
|
Just some minor but important questions: What is StreamCopy, when does that issue happen and how is that issue mitigated? Those questions are definitely coming up as the error message is just cryptic and not understanable by users. |
|
streamCopy here is used at the moment where the data is read from the PHP Input and copied into a part file on the target storage. If the target storage is not local (ex: FTP) and that storage is slow / not available / broken, it is likely that streamCopy will fail either at the beginning, or in the middle of the copy. In this case, the issue is most likely due to the target storage and not the client<->server connection. |
|
@PVince81 Thanks for the info. So why not adding this to the message as a possible reason so it would at least include some info for users? |
ghost
mentioned this pull request
|
Have raised owncloud-archive/documentation#2145 Please at least help to document the reason for this error (and also the old one in #9832). This would really help supporters trying to help other people. |
cc @DeepDiver1975 @PVince81 @dragotin
Is this what @dragotin want to have? (See #9832 (comment) )