-
Notifications
You must be signed in to change notification settings - Fork 89
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
Upload cache fixes #439
Open
pboguslawski
wants to merge
18
commits into
znuny:dev
Choose a base branch
from
ibpl:dev-IB#1113065
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Upload cache fixes #439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This mod fixes the following upload cache problems: (1) If one uploads many files (dropping files test0.bin-test9.bin in one move) races may occur and file id in DOM won't match file ids in web upload dir on server, i.e.: ``` $(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))}) test9.bin, 10 test8.bin, 8 test7.bin, 8 test6.bin, 7 test5.bin, 4 test4.bin, 5 test3.bin, 3 test2.bin, 3 test1.bin, 2 test0.bin, 1 ``` This may be fixed with patch (which unblocks updating all file ids on upload not just for one file)... ``` --- a/var/httpd/htdocs/js/Core.UI.js 2021-11-08 18:53:34.088284466 +0100 +++ b/var/httpd/htdocs/js/Core.UI.js 2021-12-16 18:41:14.090199778 +0100 @@ -697,10 +697,6 @@ $TargetObj = $ExistingItemObj.closest('tr'); - if ($TargetObj.find('a').data('file-id')) { - return; - } - $TargetObj .find('.Filetype') .text(Attachment.ContentType) ``` ...but using file ids for identification of uploaded files is not race condition resistant; to provoke race condition apply patch ``` --- a/Kernel/Modules/AjaxAttachment.pm 2021-06-19 21:59:34.379471088 +0200 +++ b/Kernel/Modules/AjaxAttachment.pm 2021-12-16 18:38:28.943925139 +0100 @@ -56,6 +56,10 @@ Param => 'Files', ); + if ($UploadStuff{Filename} eq 'test01.bin') { + sleep(1); + } + $UploadCacheObject->FormIDAddFile( FormID => $Self->{FormID}, Disposition => 'attachment', @@ -67,6 +71,10 @@ FormID => $Self->{FormID}, ); + if ($UploadStuff{Filename} eq 'test02.bin') { + sleep(2); + } + my @AttachmentData; ``` and drop files test01.bin and test02.bin (same size) in one move; after upload there will be: ``` $(".AttachmentListContainer tbody tr").filter(function() { console.log("%s, %d", $(this).find('td.Filename').text(), $(this).find('a').data('file-id'))}) test02.bin, 1 test01.bin, 1 ``` not ``` test02.bin, 2 test01.bin, 1 ``` This problem was solved by removing unnecessarry FileID attribute from upload data struct to avoid any races that may occurr between filename list and fileid list. Only filename will be used to identify items added to upload cache. (2) Double clicking trash icon to remove attachment generates two requests to server and one will cause an error. (3) Web form is not removed from upload cache dir after saving or updating template. (4) Unused upload handling code removed. Author-Change-Id: IB#1113065
Code policy cleanups. Fixes: 6e42dbb Author-Change-Id: IB#1113065
Fixes: 6e42dbb Author-Change-Id: IB#1113065
Will be moved to separate issue. Fixes: 53a7ba6 Author-Change-Id: IB#1113065
Fixes: 6e42dbb Author-Change-Id: IB#1113065
Parameter name regression fixed. Fixes: 2d608b05303537e10c75a453f941026e58c2b9eb Author-Change-Id: IB#1113065
Author-Change-Id: IB#1113065
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
This mod fixes the following upload cache problems:
(1)
If one uploads many files (dropping files test0.bin-test9.bin in one move) races may occur and file id in DOM won't match file ids in web upload dir on server, i.e.:
This may be fixed with patch (which unblocks updating all file ids on upload not just for one file)...
...but using file ids for identification of uploaded files is not race condition resistant; to provoke race condition apply patch
and drop files test01.bin and test02.bin (same size) in one move; after upload there will be:
not
This problem was solved by removing unnecessary FileID attribute from upload data struct to avoid any races that may occur between filename list and fileid list. Only filename will be used to identify items added to upload cache.
This bug is dangerous - think about uploading some confidential stuff by mistake, then removing it (disappears from UI) and sending message with different set of files than displayed in UI (i.e. with confidential stuff that user wanted to remove).
(2)
Double clicking trash icon to remove attachment generates two requests to server and one will cause an error.
(3)
Unused upload handling code removed.
Type of change
Breaking change
Removes FileID attribute from web upload cache objects to avoid races.
Additional information
Replaces: #179
Author-Change-Id: IB#1113065
Checklist