Upload: asynchronious operations #6614

ogoffart · 2018-06-25T16:18:20Z

Implements owncloud/core#31851

ogoffart · 2018-06-25T16:20:20Z

(WIP)

jnweiger · 2018-07-04T17:26:56Z

builds from this PR:

guruz · 2018-07-05T10:59:24Z

@jnweiger btw next time better build with branch name in file name:)
not that it matters much in this case

ogoffart · 2018-10-17T08:28:29Z

@PVince81 I see something was merge in the server stable10. Does that mean we should now go forward with this client side?

@ckamm : do you think this is small enough to go into 2.5.1? or should it be in master?

ckamm

If this is merged for 2.5.1 it should be in disabled state. (OC-LazyOps=false? env variable toggle?) There are some good tests but this is too much for a patch release imo.

ckamm · 2018-10-17T12:08:05Z

src/libsync/owncloudpropagator.cpp

@@ -1029,6 +1029,7 @@ void CleanupPollsJob::slotPollFinished()
            deleteLater();
            return;
        }
+        _journal->setUploadInfo(job->_item->_file, SyncJournalDb::UploadInfo());


Can PollJobs have a "there was an error, don't retry" result in any way? I'm just wondering that no other branch wipes the uploadinfo.

PollJob seems to wipe the pollinfo on non-fatal non-503

Speaking of PollInfo: It's set on startPollJob but only removed in this specific error case. And then when a sync is started, PollJobs are rerun? That means polls run indefinitely and block further syncs as long as the server replies "init" or "started"?

I think the idea is that we can continue polling for completion in case the user aborts the sync? (I think you tested that abort() case in the unittest, excellent!) PollInfos need more documentation about their purpose.

In case of error, the uploadinfo has to stay so the next sync will deal with it by retrying the final move, or use the upload info to resolve possible conflict, should the file have been updated after all (using checksum)

Yes, a server never finishing that always reply "started" will block the sync forever.
Do you think this is a problem?

UploadInfo not being wiped on failure: So if the server wants to communicate "something failed permanently, stop polling", the poll job would fail, we'd try to resend the final MOVE during the next sync and there it'd fail. Alright, that works.

Infinite polling if server replies that way: Should be ok, would be a significant server bug if that happens and working around it is complicated.

ckamm · 2018-10-17T12:14:29Z

test/testasyncop.cpp

+        });
+
+
+        auto successCallbak = [](TestCase *tc, const QNetworkRequest &request) {


minor: Callback; decide whether CallBack or Callback

ckamm · 2018-10-17T12:19:31Z

src/libsync/propagateuploadng.cpp

+            return;
+        }
+        _finished = true;
+        startPollJob(path);


How does the overall sync timeout work in the presence of polls?

The timeouts are per jobs. A particular PollJob will timeout after the normal timeout time if there is not traffic on that connection. In this case, it will be re-tried again until it succeed.

I meant to confirm that if the server needs 30 minutes to finish the upload then the sync will only finish once that last poll is done. (as it should be for 2.5.1)

PVince81 · 2018-10-17T16:13:14Z

note: there's a capability so if you deem suitable, the client could decide to enable the feature if the server does: https://github.com/owncloud/core/pull/32414/files#diff-920833889854d937ebeda804d3eb5b19R52

currently what's in 10.0.10 is in tech preview / alpha stage

ckamm · 2018-10-18T06:31:36Z

My current understanding is that this feature is enabled at the server's discretion in the first place: It looks like the client announces its capability to the server with the header, and then it either replies with the poll url or does a synchonous action.

The concern about having it default to off for the patch release is about potential bugs in the client implementation. In addition there could be a way of switching this feature off in case bugs are discovered in the server implementation.

ogoffart · 2018-10-18T08:28:00Z

@ckamm: I've updated the commit: Added comments, and disabled the behavior by default unless setting the env variable OWNCLOUD_LAZYOPS=1

@DeepDiver1975 or @PVince81 : How long does the poll url stay valid, after it once succeed. Imagine this scenario:

The client send a MOVE, the server reply with code 202 and a poll URL.
The server do the operation, the operation is finished
The client request the pull url.
The server send the "finished", but the client crashes before recieving it! (or the connection drops, or the user puts its laptop to sleep)
The client restarts and poll the URL, will that work? What if it is a month later?

Also note that the client will set the OC-LazyOps header for all uploads (non-chunking PUT or chunking MOVE).
We expect the server to be smart about it and not go into asynchronous operation if it know the operation will be fast. Going into asynchronous mode for small PUT would kill performance!

ckamm · 2018-10-18T10:02:12Z

minor: commit message says "assynchronious", should be "asynchronous"

guruz · 2018-10-25T09:58:47Z

I'm actually fine with having this in 2.5.1 if we enable this feature only by capability. (We kind-of tested this in the wild with a previous customer when it was called asynchronous PUT with poll jobs).
It could be forcible disabled by clients for testing (even if capability is on) by setting OWNCLOUD_LAZYOPS=0

In the server it's disabled by default in config.sample.php so brave server deployers need to enable it anyway in server?

if @ckamm @ogoffart agree please change milestone.

@PVince81 @ogoffart Can we still get answers to the questions above. #6614 (comment)

PVince81 · 2018-10-25T10:02:31Z

I think the server is not smart and always go to lazy mode when requested.

Not sure how long the poll URL is available, @DeepDiver1975 ?

Implements owncloud/core#31851

This was not required with 2.5 because a size of 0 was ignorted when comparing size by the csync updater, to be compatible with very old version of the database. But the we discovery will still think the file is changed if the database contains a size of 0

ogoffart · 2019-03-14T11:13:30Z

Rebased and fixed for 2.6

ogoffart requested a review from ckamm June 25, 2018 16:18

SamuAlfageme added the DO NOT MERGE YET label Jun 26, 2018

ogoffart changed the title ~~Upload: assynchronious operations~~ Upload: asynchronious operations Sep 14, 2018

ogoffart force-pushed the amd branch from 82ff1b5 to 05df6a7 Compare October 17, 2018 08:45

ogoffart changed the base branch from master to 2.5 October 17, 2018 08:46

ckamm reviewed Oct 17, 2018

View reviewed changes

ogoffart force-pushed the amd branch from 05df6a7 to f68183c Compare October 18, 2018 08:18

ckamm approved these changes Oct 18, 2018

View reviewed changes

guruz added this to the 2.6.0 milestone Oct 25, 2018

ogoffart mentioned this pull request Feb 8, 2019

Vfs adjustments for deeper shell integration #7017

Merged

ogoffart changed the base branch from 2.5 to master March 14, 2019 09:39

ogoffart added 2 commits March 14, 2019 10:41

Upload: asynchronious operations

ef3b9de

Implements owncloud/core#31851

ogoffart force-pushed the amd branch from f68183c to 4684fa7 Compare March 14, 2019 11:11

ogoffart requested a review from ckamm March 14, 2019 11:11

ogoffart removed the DO NOT MERGE YET label Mar 14, 2019

ckamm approved these changes Mar 15, 2019

View reviewed changes

guruz merged commit 269a7b6 into master Mar 20, 2019

guruz deleted the amd branch March 20, 2019 11:30

michaelstingl mentioned this pull request Apr 13, 2020

Decide on HTTP chunked-data transfer protocol cs3org/reva#290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload: asynchronious operations #6614

Upload: asynchronious operations #6614

ogoffart commented Jun 25, 2018

ogoffart commented Jun 25, 2018

jnweiger commented Jul 4, 2018

guruz commented Jul 5, 2018

ogoffart commented Oct 17, 2018

ckamm left a comment

ckamm Oct 17, 2018

ckamm Oct 17, 2018

ogoffart Oct 18, 2018

ogoffart Oct 18, 2018

ckamm Oct 18, 2018

ckamm Oct 17, 2018

ckamm Oct 17, 2018

ogoffart Oct 18, 2018

ckamm Oct 18, 2018

PVince81 commented Oct 17, 2018

ckamm commented Oct 18, 2018

ogoffart commented Oct 18, 2018

ckamm commented Oct 18, 2018

guruz commented Oct 25, 2018

PVince81 commented Oct 25, 2018

ogoffart commented Mar 14, 2019

		});


		auto successCallbak = [](TestCase *tc, const QNetworkRequest &request) {

Upload: asynchronious operations #6614

Upload: asynchronious operations #6614

Conversation

ogoffart commented Jun 25, 2018

ogoffart commented Jun 25, 2018

jnweiger commented Jul 4, 2018

guruz commented Jul 5, 2018

ogoffart commented Oct 17, 2018

ckamm 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

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

PVince81 commented Oct 17, 2018

ckamm commented Oct 18, 2018

ogoffart commented Oct 18, 2018

ckamm commented Oct 18, 2018

guruz commented Oct 25, 2018

PVince81 commented Oct 25, 2018

ogoffart commented Mar 14, 2019