[POC] Feature/introduce amd to MOVE #31851

DeepDiver1975 · 2018-06-21T09:07:13Z

Description

We want to avoid situations where the clients have to wait for the long MOVE operation on assembly to finish. The connection can be terminated by any reason before the server sends the response back to the client. And after that the client has no capability to query for the status of the assembly.

AMD - Asynchronous Method Dispatch
We return the http status code 202 right after we received the MOVE request. The client can then terminate the connection and knows that the server is working on it.
The implementation uses php's register_callback functionality to process the real MOVE after the response is sent to the client. register_callback can not be terminated by the system - we should be pretty save on this one ....
The 202 response will hold a header 'OC-JobStatus-Location' which will point the client to the job status endpoint
Job Status Endpoint
The job status endpoint is a DAV resource in the following format: /dav/job-status/{$userId}/{$this->jobId}. It will return a json object holding the job status information.

Property	Description
status	init, started, finished, error
fileId	the file id of the target file - set only once the MOVE was successful
ETag	the ETag of the file after MOVE on the target location - set only once the MOVE was successfull
errorCode	http error code - in case there was an error
errorMessage	there error message - in case of an error

Server Side Event on the job status endpoint
The client can request the job status in simple request/response manner or request an event stream following the Server Side Event protocol. To initiate SSE handling the query parameter sse=1 is appended to the GET request. As a result the server will not terminate the connection but keep it open and stream job status changes to the client. The client is asked to close the connection once the status is success or error
Capabilities and LazyOps header
The server will announce to the clients via the capabilities API if this new mode of operations is available or not. A system config value is necessary to allow the admin to turn this feature explicitly on.

Furthermore in order not to break regular WebDAV client an additional http header is to be sent from the client to the server so that the server knows if this operation mode shall be used or not.

Todo

add capability
add COPY support
detailed implementation to run MOVE pre flight checks before sending 202

Motivation and Context

Introduce a more generic async mechanism

How Has This Been Tested?

move

deepdiver@deepdiver:~$ curl -X MOVE  http://192.168.178.44:8080/remote.php/dav/files/admin/welcome.txt -uadmin -H 'OC-LazyOps: true' -H 'Destination: http://192.168.178.44:8080/remote.php/dav/files/admin/x/welcome.txt' -v
Enter host password for user 'admin':
*   Trying 192.168.178.44...
* TCP_NODELAY set
* Connected to 192.168.178.44 (192.168.178.44) port 8080 (#0)
* Server auth using Basic with user 'admin'
> MOVE /remote.php/dav/files/admin/welcome.txt HTTP/1.1
> Host: 192.168.178.44:8080
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/7.60.0
> Accept: */*
> OC-LazyOps: true
> Destination: http://192.168.178.44:8080/remote.php/dav/files/admin/x/welcome.txt
> 
< HTTP/1.1 202 Accepted
< Host: 192.168.178.44:8080
< Date: Thu, 21 Jun 2018 08:44:43 +0000
< Connection: close
< X-Powered-By: PHP/7.1.18-1+ubuntu18.04.1+deb.sury.org+1
< Expires: Thu, 19 Nov 1981 08:52:00 GMT
< Cache-Control: no-store, no-cache, must-revalidate
< Pragma: no-cache
< Set-Cookie: oc_sessionPassphrase=YABFWiAVwlohlGBpO8j3dv9zD%2FZhAesNpb482kp8Xt9iESpBU5tVGJuvrlCwvfWH1dLz23CknJcfzaFYgxzsmfZ6gMmtAkgBKpI4IlX8NKfPf9MVBShNTtzswKsaGoG%2B; path=/; HttpOnly
< X-XSS-Protection: 1; mode=block
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Robots-Tag: none
< X-Download-Options: noopen
< X-Permitted-Cross-Domain-Policies: none
< Content-Security-Policy: default-src 'none';
< Set-Cookie: oc9fy8f2gwa8=n18gf5i45fkdkka1ual4c8mgvt; path=/; HttpOnly
< Set-Cookie: cookie_test=test; expires=Thu, 21-Jun-2018 09:44:40 GMT; Max-Age=3600
< Connection: close
< OC-Location: /remote.php/dav/admin/queue/d3c011a2-e640-4b94-bd7d-897aa99e9585
< Content-type: text/html; charset=UTF-8
< 
* Closing connection 0

Query for the status:

deepdiver@deepdiver:~$ curl -v -uadmin http://192.168.178.44:8080/remote.php/dav/queue/admin/d3c011a2-e640-4b94-bd7d-897aa99e9585
Enter host password for user 'admin':
*   Trying 192.168.178.44...
* TCP_NODELAY set
* Connected to 192.168.178.44 (192.168.178.44) port 8080 (#0)
* Server auth using Basic with user 'admin'
> GET /remote.php/dav/queue/admin/d3c011a2-e640-4b94-bd7d-897aa99e9585 HTTP/1.1
> Host: 192.168.178.44:8080
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/7.60.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< Host: 192.168.178.44:8080
< Date: Thu, 21 Jun 2018 09:00:06 +0000
< Connection: close
< X-Powered-By: PHP/7.1.18-1+ubuntu18.04.1+deb.sury.org+1
< Expires: Thu, 19 Nov 1981 08:52:00 GMT
< Cache-Control: no-store, no-cache, must-revalidate
< Pragma: no-cache
< Set-Cookie: oc_sessionPassphrase=PFsuMhAl1oNSkKVHkAKY2vxSPKtJ%2BVBMMWx1zf6%2B6O%2FQU4PBz1%2BEssbXozILMCwXwNanx%2BnuaSSooLekzD2%2BOq46tDOCN9ZEjW412Ot2s%2FD55hKzLAAfFtGqFNVlBr7Z; path=/; HttpOnly
< X-XSS-Protection: 1; mode=block
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Robots-Tag: none
< X-Download-Options: noopen
< X-Permitted-Cross-Domain-Policies: none
< Content-Security-Policy: default-src 'none';
< Set-Cookie: oc9fy8f2gwa8=8fbdm6ijnlah8162lc7lanf7ba; path=/; HttpOnly
< Set-Cookie: cookie_test=test; expires=Thu, 21-Jun-2018 10:00:06 GMT; Max-Age=3600
< Content-Type: application/json
< ETag: "e12309511a0155a316c6223c64511ba15b217a1a"
< Content-Length: 99
< OC-ETag: "e12309511a0155a316c6223c64511ba15b217a1a"
< 
* Closing connection 0
{"status":"finished","fileId":"00003061oc9fy8f2gwa8","ETag":"\"f3610808373e6aad584d69b4a0a068f5\""}

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2018-06-21T09:46:09Z

Codecov Report

Merging #31851 into master will decrease coverage by 0.37%.
The diff coverage is 77.71%.

@@             Coverage Diff              @@
##             master   #31851      +/-   ##
============================================
- Coverage        64%   63.62%   -0.38%     
- Complexity    18578    18594      +16     
============================================
  Files          1171     1176       +5     
  Lines         69912    69772     -140     
  Branches       1267     1264       -3     
============================================
- Hits          44746    44393     -353     
- Misses        24796    25010     +214     
+ Partials        370      369       -1

Flag	Coverage Δ	Complexity Δ
#javascript	`52.59% <100%> (-0.26%)`	`0 <0> (ø)`
#phpunit	`64.88% <77.32%> (-0.4%)`	`18594 <39> (+16)`

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Connector/Sabre/Server.php	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...s/dav/appinfo/Migrations/Version20180622095921.php	`0% <0%> (ø)`	`2 <2> (?)`
apps/dav/lib/Capabilities.php	`0% <0%> (ø)`	`3 <1> (+2)`	⬆️
apps/dav/lib/JobStatus/Home.php	`100% <100%> (ø)`	`5 <5> (?)`
apps/dav/lib/RootCollection.php	`100% <100%> (ø)`	`1 <0> (ø)`	⬇️
apps/dav/lib/Server.php	`52.98% <100%> (+1.55%)`	`25 <0> (+1)`	⬆️
core/js/files/client.js	`80.99% <100%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/JobStatus/Entity/JobStatusMapper.php	`100% <100%> (ø)`	`2 <2> (?)`
apps/dav/lib/JobStatus/JobStatus.php	`100% <100%> (ø)`	`7 <7> (?)`
apps/dav/lib/JobStatus/RootCollection.php	`40% <40%> (ø)`	`2 <2> (?)`
... and 116 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d743ffd...5042ce9. Read the comment docs.

PVince81 · 2018-06-21T10:53:30Z

We cannot re-use the Location header

try with "Content-Location" then. we did that for other DAV APIs

PVince81 · 2018-06-21T11:01:55Z

it's not strictly the entity's location, so maybe call it "Queue-Location" or "OC-Queue-Location" ?

PVince81 · 2018-06-21T11:04:06Z

apps/dav/lib/DAV/LazyOpsPlugin.php

+	}
+
+	public function httpMove(RequestInterface $request, ResponseInterface $response) {
+		if (!$request->getHeader('OC-LazyOps')) {


we could call this "Queue" or something referring to the queue, so we have the same terminology everywhere

absolutly - I'm not yet fully convincent with the resource name 'queue' as well - since it is not a queue ....

PVince81

interesting concept

The queue could also be used to track stray/stuck uploads and clean up chunks afterwards. This would require using the queue earlier when dealing with chunks. Or needs a completely separate mechanism.

PVince81 · 2018-06-21T11:04:18Z

apps/dav/lib/DAV/LazyOpsPlugin.php

+
+		$response->setStatus(202);
+		$response->addHeader('Connection', 'close');
+		$response->addHeader('OC-Location', $location);


OC-Queue-Location ?

PVince81 · 2018-06-21T11:06:07Z

apps/dav/lib/DAV/LazyOpsPlugin.php

+			]);
+			$this->server->emit('method:MOVE', [$request, $responseDummy]);
+
+			$this->setJobStatus([


if we can store the final file name in there we could solve a lot of problems, like the fact that encryption relies on the part file name and uses magic extraction to find the final file name... here it could just read from the upload queue

speaking of which... does this work with user-key encryption ? cron doesn't have the user's private key so cannot encrypt...

the execution is not done in cron - but in the original client request.

The difference is that we defer the execution to PHP's shutdown handler which is called after the response is sent back to the client

ogoffart · 2018-06-21T14:24:59Z

Good. This more or less follow the design that was already done in the client and documented in #12097 so implementation in the client can re-use that. (although that code sort of bit-rotted and was not ported to new chunking)

tomneedham · 2018-06-21T15:42:58Z

apps/dav/lib/Queue/LongPollPlugin.php

+				\sleep(1);
+			}
+
+			if (connection_aborted()) {


butonic · 2018-06-22T06:56:25Z

apps/dav/lib/DAV/LazyOpsPlugin.php

+	private $jobId;
+
+	public static function getQueueInfo(string $userId, string $jobId) {
+		return \OC::$server->getConfig()->getUserValue($userId, 'dav', "lazy-ops-job.$jobId", null, false);


I assume you are going to replace this with a proper queue table ...

or use a file property ... config is really not the place for this ....

sure - this was for the sake of fast POC

Could you use the existing oc_jobs? Insert as a 'already reserved' job, and add a status column?

Could you use the existing oc_jobs? Insert as a 'already reserved' job, and add a status column?

hmmmm - let me think about this ....

I don't think so - there is no way to select the job status by it's id where the is is a uuid ....
... and also the processing logic will not work.

DeepDiver1975 · 2018-06-22T07:22:49Z

In order to detect that the connection was closed by the client the server needs to send something over the wire.

DeepDiver1975 · 2018-06-22T09:09:22Z

In order to detect that the connection was closed by the client the server needs to send something over the wire.

handled by the used library https://github.com/hhxsv5/php-sse/releases/tag/1.1.4

Implements owncloud/core#31851

micbar · 2018-08-17T14:18:34Z

@PVince81 needs backport for 10, see https://github.com/owncloud/enterprise/issues/2480

DeepDiver1975 · 2018-08-17T14:39:28Z

@PVince81 needs backport for 10, see owncloud/enterprise#2480

We have to analyse the impact of SSE to the system. Following the semver definitions this is nothing to be added to 10.0.x but to 10.1

PVince81 · 2018-08-27T10:05:46Z

apps/dav/lib/Server.php

@@ -86,6 +90,17 @@ public function __construct(IRequest $request, $baseUri) {
 		$tree = new \OCA\DAV\Tree($root);
 		$this->server = new \OCA\DAV\Connector\Sabre\Server($tree);

+		$config = \OC::$server->getConfig();
+		if ($config->getSystemValue('dav.enable.async', true)) {


for this backport version I think we should disable this by default ?

PVince81 · 2018-09-04T19:21:08Z

the stable10 simplified version got merged. Here's its master port: #32576

Implements owncloud/core#31851

DeepDiver1975 requested review from IljaN, PVince81, butonic and patrickjahns June 21, 2018 09:08

DeepDiver1975 self-assigned this Jun 21, 2018

DeepDiver1975 added the 2 - Developing label Jun 21, 2018

DeepDiver1975 added this to the development milestone Jun 21, 2018

DeepDiver1975 mentioned this pull request Jun 21, 2018

Introduce IShutdownManager which allows us to order the shutdown func… #31857

Merged

9 tasks

PVince81 reviewed Jun 21, 2018

View reviewed changes

tomneedham reviewed Jun 21, 2018

View reviewed changes

butonic reviewed Jun 22, 2018

View reviewed changes

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from a6192ed to 0266157 Compare June 22, 2018 09:34

DeepDiver1975 mentioned this pull request Jun 22, 2018

[POC]Introduce AMD style MOVE operation #31758

Closed

9 tasks

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch 2 times, most recently from d3783cd to 805a99b Compare June 25, 2018 07:13

DeepDiver1975 changed the title ~~[POC] Feature/introduce amd to chunk move 2~~ [POC] Feature/introduce amd to MOVE Jun 25, 2018

PVince81 mentioned this pull request Jun 25, 2018

Implement async file upload in web UI upload #31880

Closed

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from 805a99b to 1e3d5bb Compare June 25, 2018 10:19

ogoffart added a commit to owncloud/client that referenced this pull request Jun 25, 2018

Upload: assynchronious operations

82ff1b5

Implements owncloud/core#31851

ogoffart mentioned this pull request Jun 25, 2018

Upload: asynchronious operations owncloud/client#6614

Merged

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch 3 times, most recently from f3bb9fd to aab4804 Compare June 27, 2018 14:32

guruz mentioned this pull request Jun 29, 2018

Local DELETE on server-locked resource owncloud/client#6513

Closed

PVince81 mentioned this pull request Jun 29, 2018

Resuming chunked uploads owncloud/android#2070

Open

ownclouders added the status/STALE label Jul 6, 2018

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from aab4804 to 57ce13c Compare July 6, 2018 07:55

ownclouders removed the status/STALE label Jul 6, 2018

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from 57ce13c to c1c2671 Compare July 6, 2018 10:40

ownclouders added the status/STALE label Jul 14, 2018

ownclouders removed the status/STALE label Aug 17, 2018

butonic force-pushed the feature/introduce-amd-to-chunk-move-2 branch from c1c2671 to e2ad740 Compare August 21, 2018 13:51

DeepDiver1975 added 4 commits August 22, 2018 16:48

composer require hhxsv5/php-sse

643a450

Introduce AMD style MOVE operation

18db44b

Add tests for SSE plugin

2e9e1fa

Add Capability

6c3d738

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from e2ad740 to 5042ce9 Compare August 22, 2018 14:48

Use async in files app

cef6bdc

DeepDiver1975 force-pushed the feature/introduce-amd-to-chunk-move-2 branch from 5042ce9 to cef6bdc Compare August 22, 2018 14:52

DeepDiver1975 mentioned this pull request Aug 23, 2018

Backport/31851 async put #32414

Merged

3 tasks

PVince81 reviewed Aug 27, 2018

View reviewed changes

PVince81 mentioned this pull request Sep 4, 2018

[master] Async put with poll #32576

Merged

individual-it mentioned this pull request Sep 6, 2018

allow to slowdown dav requests owncloud/testing#21

Merged

1 task

ogoffart added a commit to owncloud/client that referenced this pull request Oct 17, 2018

Upload: assynchronious operations

05df6a7

Implements owncloud/core#31851

DeepDiver1975 closed this Oct 17, 2018

DeepDiver1975 deleted the feature/introduce-amd-to-chunk-move-2 branch October 17, 2018 08:52

ogoffart added a commit to owncloud/client that referenced this pull request Oct 18, 2018

Upload: assynchronious operations

f68183c

Implements owncloud/core#31851

ogoffart added a commit to owncloud/client that referenced this pull request Mar 14, 2019

Upload: asynchronious operations

ef3b9de

Implements owncloud/core#31851

guruz pushed a commit to owncloud/client that referenced this pull request Mar 20, 2019

Upload: asynchronious operations

3093ef5

Implements owncloud/core#31851

lock bot locked as resolved and limited conversation to collaborators Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Feature/introduce amd to MOVE #31851

[POC] Feature/introduce amd to MOVE #31851

DeepDiver1975 commented Jun 21, 2018 •

edited

Loading

codecov bot commented Jun 21, 2018 •

edited

Loading

PVince81 commented Jun 21, 2018

PVince81 commented Jun 21, 2018

PVince81 Jun 21, 2018

DeepDiver1975 Jun 21, 2018

PVince81 left a comment

PVince81 Jun 21, 2018

PVince81 Jun 21, 2018

DeepDiver1975 Jun 21, 2018

ogoffart commented Jun 21, 2018

tomneedham Jun 21, 2018

butonic Jun 22, 2018

butonic Jun 22, 2018

DeepDiver1975 Jun 22, 2018

tomneedham Jun 22, 2018

DeepDiver1975 Jun 22, 2018

DeepDiver1975 Jun 22, 2018

DeepDiver1975 commented Jun 22, 2018

DeepDiver1975 commented Jun 22, 2018

micbar commented Aug 17, 2018

DeepDiver1975 commented Aug 17, 2018

PVince81 Aug 27, 2018

PVince81 commented Sep 4, 2018

[POC] Feature/introduce amd to MOVE #31851

[POC] Feature/introduce amd to MOVE #31851

Conversation

DeepDiver1975 commented Jun 21, 2018 • edited Loading

Description

Todo

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Jun 21, 2018 • edited Loading

Codecov Report

PVince81 commented Jun 21, 2018

PVince81 commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogoffart commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Jun 22, 2018

DeepDiver1975 commented Jun 22, 2018

micbar commented Aug 17, 2018

DeepDiver1975 commented Aug 17, 2018

Choose a reason for hiding this comment

PVince81 commented Sep 4, 2018

DeepDiver1975 commented Jun 21, 2018 •

edited

Loading

codecov bot commented Jun 21, 2018 •

edited

Loading