locking for chunked dav upload #17104

icewind1991 · 2015-06-23T11:53:40Z

Change the locking handler code to work directly on the storage and lock the upload target while we're assembling the chunks.

cc @DeepDiver1975 @PVince81

PVince81 · 2015-06-23T12:04:43Z

Dangerous change that late in the release, it does not only affect locking.

It seems you aren't firing any hooks ?

I'd rather have this in 8.2.

Locking already works with chunking, it just doesn't wrap everything in a bigger transaction yet. This could be done in 8.2 instead.

icewind1991 · 2015-06-23T12:54:14Z

file_assemble emits hooks

MorrisJobke · 2015-06-24T09:03:00Z

I'd rather have this in 8.2.

cc @DeepDiver1975

DeepDiver1975 · 2015-06-24T09:42:03Z

Locking already works with chunking, it just doesn't wrap everything in a bigger transaction yet. This could be done in 8.2 instead.

With and without this change the whole assembly process (which is the one which eats time) is locked - so I guess this is not a critical/required change.

@icewind1991 do I miss anything?

What we need is a locking mechanism for the whole chunked upload procedure?
But this is something we can keep for 8.2.

icewind1991 · 2015-06-24T10:34:57Z

Without this change the assembly itself is locked but only on the part file path and the other related operations (cache updates, touch, etc) are not locked.

DeepDiver1975 · 2015-06-24T10:36:36Z

and the other related operations (cache updates, touch, etc) are not locked.

understood - what is the risk?

MorrisJobke · 2015-06-26T07:49:42Z

@icewind1991 Ping

icewind1991 · 2015-06-26T11:20:24Z

I don't think the risk is high, in most cases the only risk is between assembling the file in a .part and renaming it to the final target

PVince81 · 2015-06-26T12:30:58Z

When I asked about risk, I was talking about risk of regression.
Anyway, this will need retesting chunked upload which can hopefully be done by smashbox.

PVince81 · 2015-06-26T12:32:27Z

The following things need to be tested:

TEST: chunk upload still works
TEST: chunk upload when file is already locked: should fail with 423 Locked
TEST: other process accesses file while chunk upload locked file: fail with 423 Locked
TEST: pre-hooks called (versions app?)
TEST: post-hooks called (versions app?

PVince81 · 2015-06-26T12:33:18Z

Also missing:

TODO: add unit test to check for locks in pre and post hooks

Last week I already added tests for the non-chunking case, hopefully these can be reused.

DeepDiver1975 · 2015-06-26T12:35:03Z

When I asked about risk, I was talking about risk of regression.
Anyway, this will need retesting chunked upload which can hopefully be done by smashbox.

@PVince81 sure - but my question was explicit on the risk of NOT having this change in.

As the risk is not that high I tend to move this to 8.2.

@PVince81 agree?
@icewind1991 agree?

THX

PVince81 · 2015-06-26T12:37:58Z

Yes, let's make this 8.2.
We still already have enough other issues to attend to for 8.1

icewind1991 · 2015-06-26T14:39:49Z

8.2 is fine by me

PVince81 · 2015-07-06T09:35:21Z

@icewind1991 please rebase and add unit tests.
Have a look how I did it https://github.com/owncloud/core/blob/master/tests/lib/connector/sabre/file.php#L305 for single file upload.

PVince81 · 2015-07-31T09:35:20Z

Please make sure folder size propagation still works, see #18006

PVince81 · 2015-09-01T13:46:05Z

@icewind1991 did you try reproducing the issue ?

scrutinizer-notifier · 2015-09-14T13:33:57Z

A new inspection was created.

PVince81 · 2015-10-12T12:27:41Z

Had a quick test with the sync client to verify that chunk uploads are still creating versions. This confirms that hooks are fired properly.

👍

PVince81 · 2015-10-12T12:27:56Z

(not too happy to have such huge change that late, but fine)

DeepDiver1975 · 2015-10-12T12:51:21Z

(not too happy to have such huge change that late, but fine)

You are right - there is no real need for this (as in fixes sev1 or sev2)

8.2.1 or 9.0 ?

MorrisJobke · 2015-10-13T09:06:39Z

8.2.1 or 9.0 ?

I would say 9.0, because backporting such stuff in a point release is really tricky and can go horribly wrong. Having this in early 9.0 will be much better.

MorrisJobke · 2015-10-13T09:07:01Z

cc @karlitschek @cmonteroluque

ghost · 2015-10-13T11:33:44Z

OK, moving to 9.0

karlitschek · 2015-10-13T13:38:25Z

yes. too risky

PVince81 · 2015-10-20T11:52:17Z

Needs a second reviewer @nickvergessen @DeepDiver1975

MorrisJobke · 2015-10-20T13:17:48Z

apps/dav/tests/unit/connector/sabre/requesttest/uploadtest.php

@@ -43,6 +45,34 @@ public function testUploadOverWrite() {
 		$this->assertEquals(3, $info->getSize());
 	}

+	/**
+	 * @expectedException \OC\Connector\Sabre\Exception\FileLocked


I guess this needs to be adjusted to the new location inside apps/dav

icewind1991 · 2015-10-22T13:00:05Z

@MorrisJobke @DeepDiver1975 can this be merged?

MorrisJobke · 2015-10-22T14:04:12Z

@MorrisJobke @DeepDiver1975 can this be merged?

Letting Smashbox run against this? ;)

cc @DeepDiver1975 @nickvergessen

nickvergessen · 2015-10-26T13:18:02Z

Smashed: ✅

icewind1991 · 2015-10-27T12:26:09Z

@DeepDiver1975 @MorrisJobke merge?

MorrisJobke · 2015-10-27T15:42:44Z

I also did some tests with cadaver as 3rdparty client. Everything still works 👍

locking for chunked dav upload

icewind1991 added the feature:locking label Jun 23, 2015

DeepDiver1975 added this to the 8.1-current milestone Jun 23, 2015

PVince81 mentioned this pull request Jun 23, 2015

Issues to solve with locking #16664

Closed

46 tasks

SergioBertolinSG mentioned this pull request Jun 23, 2015

[Experimental files locking] Desktop client doesn't lock folders. owncloud/client#3338

Closed

MorrisJobke added the 3 - To Review label Jun 24, 2015

DeepDiver1975 modified the milestones: 8.2-next, 8.1-current Jun 26, 2015

PVince81 mentioned this pull request Jul 3, 2015

[WIP] introduction of interface IChunkHandler and a very first stupid implemen... #12160

Closed

MorrisJobke added 2 - Developing and removed 3 - To Review labels Jul 6, 2015

PVince81 mentioned this pull request Jul 31, 2015

Folder size propagation not triggered by Webdav upload #18006

Closed

icewind1991 force-pushed the chunked-upload-locking branch from dc5b825 to faa59c0 Compare September 14, 2015 12:49

ghost modified the milestones: 9.0-next, 8.2-current Oct 13, 2015

icewind1991 added 5 commits October 20, 2015 14:14

locking for chunked dav upload

23eaf27

Add tests for uploading to locked files

21d0267

Adjust for wide locking

ddc8749

fix locking in tests

760335c

remove locking for chunks

283798a

icewind1991 force-pushed the chunked-upload-locking branch from ebcdccf to 283798a Compare October 20, 2015 12:16

MorrisJobke reviewed Oct 20, 2015
View reviewed changes

adjust tests for new dav classes

021ed8b

MorrisJobke added 4 - To release and removed 3 - To Review labels Oct 27, 2015

icewind1991 added a commit that referenced this pull request Oct 27, 2015

Merge pull request #17104 from owncloud/chunked-upload-locking

c309193

locking for chunked dav upload

icewind1991 merged commit c309193 into master Oct 27, 2015

icewind1991 deleted the chunked-upload-locking branch October 27, 2015 15:58

icewind1991 mentioned this pull request Mar 22, 2016

File Is Loked #22591

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

locking for chunked dav upload #17104

locking for chunked dav upload #17104

icewind1991 commented Jun 23, 2015

PVince81 commented Jun 23, 2015

icewind1991 commented Jun 23, 2015

MorrisJobke commented Jun 24, 2015

DeepDiver1975 commented Jun 24, 2015

icewind1991 commented Jun 24, 2015

DeepDiver1975 commented Jun 24, 2015

MorrisJobke commented Jun 26, 2015

icewind1991 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

DeepDiver1975 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

icewind1991 commented Jun 26, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 31, 2015

PVince81 commented Sep 1, 2015

scrutinizer-notifier commented Sep 14, 2015

PVince81 commented Oct 12, 2015

PVince81 commented Oct 12, 2015

DeepDiver1975 commented Oct 12, 2015

MorrisJobke commented Oct 13, 2015

MorrisJobke commented Oct 13, 2015

ghost commented Oct 13, 2015

karlitschek commented Oct 13, 2015

PVince81 commented Oct 20, 2015

MorrisJobke Oct 20, 2015

icewind1991 Oct 20, 2015

icewind1991 commented Oct 22, 2015

MorrisJobke commented Oct 22, 2015

nickvergessen commented Oct 26, 2015

icewind1991 commented Oct 27, 2015

MorrisJobke commented Oct 27, 2015

locking for chunked dav upload #17104

locking for chunked dav upload #17104

Conversation

icewind1991 commented Jun 23, 2015

PVince81 commented Jun 23, 2015

icewind1991 commented Jun 23, 2015

MorrisJobke commented Jun 24, 2015

DeepDiver1975 commented Jun 24, 2015

icewind1991 commented Jun 24, 2015

DeepDiver1975 commented Jun 24, 2015

MorrisJobke commented Jun 26, 2015

icewind1991 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

DeepDiver1975 commented Jun 26, 2015

PVince81 commented Jun 26, 2015

icewind1991 commented Jun 26, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 31, 2015

PVince81 commented Sep 1, 2015

scrutinizer-notifier commented Sep 14, 2015

PVince81 commented Oct 12, 2015

PVince81 commented Oct 12, 2015

DeepDiver1975 commented Oct 12, 2015

MorrisJobke commented Oct 13, 2015

MorrisJobke commented Oct 13, 2015

ghost commented Oct 13, 2015

karlitschek commented Oct 13, 2015

PVince81 commented Oct 20, 2015

MorrisJobke Oct 20, 2015

Choose a reason for hiding this comment

icewind1991 Oct 20, 2015

Choose a reason for hiding this comment

icewind1991 commented Oct 22, 2015

MorrisJobke commented Oct 22, 2015

nickvergessen commented Oct 26, 2015

icewind1991 commented Oct 27, 2015

MorrisJobke commented Oct 27, 2015