Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scan the file in the write lock when uploading over dav #22290

Merged
merged 2 commits into from Feb 11, 2016

Conversation

icewind1991
Copy link
Contributor

Currently there is a slight chance that a different process might trigger the scan before the upload process got the time do to it.

Note that we can do this within the exclusive lock only since the scan trigger from Updater->update explicitly does not acquire another read lock

Should probably be backported to 8.2.x once properly tested

cc @PVince81 @schiesbn

@icewind1991 icewind1991 added this to the 9.0-current milestone Feb 10, 2016
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @PVince81, @DeepDiver1975 and @bartv2 to be potential reviewers

@PVince81
Copy link
Contributor

That seems to make sense.

How about the other code path, the one that does chunking ? Does it also need a similar fix ?

@icewind1991
Copy link
Contributor Author

also fixed the chunked upload code path

@PVince81
Copy link
Contributor

Not sure if bad luck...

SUMMARY:smash.:running lib/test_basicSync.py in /home/vincent/work/smashdir/test_basicSync as test testset #1 {'basicSync_filesizeKB': 5000, 'basicSync_rmLocalStateDB': False}
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
INFO:requests.packages.urllib3.connectionpool:Starting new HTTP connection (1): 127.0.0.1
2016-02-10 18:34:27,305 - ERROR - creator - expected 0 got 1 deleted files error_check(len(expected_deleted_files) == len(actual_deleted_files), "expected %d got %d deleted files"%(len(expected_deleted_files),len(actual_deleted_files))) failed in expect_deleted_files() ["/home/vincent/work/workspace/smashbox/lib/test_basicSync.py" at line 70]
2016-02-10 18:34:27,305 - ERROR - loser - expected 0 got 1 deleted files error_check(len(expected_deleted_files) == len(actual_deleted_files), "expected %d got %d deleted files"%(len(expected_deleted_files),len(actual_deleted_files))) failed in expect_deleted_files() ["/home/vincent/work/workspace/smashbox/lib/test_basicSync.py" at line 70]
2016-02-10 18:34:27,305 - ERROR - checker - expected 0 got 1 deleted files error_check(len(expected_deleted_files) == len(actual_deleted_files), "expected %d got %d deleted files"%(len(expected_deleted_files),len(actual_deleted_files))) failed in expect_deleted_files() ["/home/vincent/work/workspace/smashbox/lib/test_basicSync.py" at line 70]
2016-02-10 18:34:28,260 - ERROR - loser - 1 error(s) reported
2016-02-10 18:34:28,260 - ERROR - checker - 1 error(s) reported
2016-02-10 18:34:28,261 - ERROR - creator - 1 error(s) reported
SUMMARY:smash.:Elapsed time: 35s (0:00:35.762834)
CRITICAL:smash.:Aborting run -- non-zero exit code (2)
fullsmash.sh  222.94s user 131.12s system 15% cpu 38:52.55 total

@PVince81
Copy link
Contributor

I ran test_basicSync a few times again, alone (not will fullsmash) and it went through... could be the known random fail.

@PVince81
Copy link
Contributor

Ok, it seems to work now. @nickvergessen confirmed to me that this test randomly fails.

So I think this PR is fine 👍

Second reviewer @nickvergessen @schiesbn @nickvergessen @DeepDiver1975 ?

@PVince81
Copy link
Contributor

@butonic @MorrisJobke potential fix for the scanning concurrency issue

@MorrisJobke
Copy link
Contributor

Upload and sync still works 👍

@MorrisJobke
Copy link
Contributor

Should probably be backported to 8.2.x once properly tested

cc @karlitschek

DeepDiver1975 added a commit that referenced this pull request Feb 11, 2016
scan the file in the write lock when uploading over dav
@DeepDiver1975 DeepDiver1975 merged commit 26939a2 into master Feb 11, 2016
@DeepDiver1975 DeepDiver1975 deleted the dav-upload-scan-in-lock branch February 11, 2016 13:02
@MorrisJobke
Copy link
Contributor

@icewind1991 Could you please prepare the backport PR

@icewind1991
Copy link
Contributor Author

8.2: #22325

@karlitschek
Copy link
Contributor

great fix. Please backport! 👍

@lock
Copy link

lock bot commented Aug 7, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants