Review Request 125613: Race condition and error notification loss in ListJob

[Alberto Jiménez Ruiz]

> On Oct. 19, 2015, 7:28 a.m., David Faure wrote:
> > kio/kio/job.cpp, line 2671
> > <https://git.reviewboard.kde.org/r/125613/diff/3/?file=411228#file411228line2671>
> >
> >     Same question here. As the comment says, "the result is still OK", which is why I didn't set an error code here. Please explain why this is needed.
> 
> Alberto Jiménez Ruiz wrote:
>     The comment exactly says: (If we can't list a subdir, the result is still ok).
>     
>     I interpret it like the result is not ok if the error is not about being unable to list a directory.
> 
> David Faure wrote:
>     Which other type of error would there be, when trying to list a subdirectory?
>     
>     Whatever the error is, the result is that we were not able to list that directory.
>     
>     (whether it's because of permission errors, or because it was deleted during the listing by the user (race condition), or ... not sure what else could happen, really)
>     
>     I think there's no reason for testing for a specific error code, and I also think we don't want to setError, since "it's still OK".

That's fine, throwing just a warning signal should be fine.


- Alberto


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125613/#review87048
-----------------------------------------------------------


On Oct. 19, 2015, 9:43 a.m., Alberto Jiménez Ruiz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125613/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2015, 9:43 a.m.)
> 
> 
> Review request for kdelibs, Albert Astals Cid and David Faure.
> 
> 
> Bugs: 333436
>     http://bugs.kde.org/show_bug.cgi?id=333436
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Race condition and error notification loss in ListJob
> 
> 
> Diffs
> -----
> 
>   kio/kio/job.cpp 91712e3 
>   kio/kio/jobclasses.h d771cfe 
>   kio/tests/jobtest.h d3c552e 
>   kio/tests/jobtest.cpp ee2677a 
> 
> Diff: https://git.reviewboard.kde.org/r/125613/diff/
> 
> 
> Testing
> -------
> 
> Update 1, added unittest
> Changed condition of listjob slotresult finishing (When executed as a synchronous job, gets stuck in a eventloop)
> Added setError to copyjob.
> 
> 
> Tests done on Kubuntu 15.10:
> 
> With this folder structure in ~:
> src
> ######c (Folder)
> ##############b (Folder chmod'ed to 700 and owned by root)
> #######################d (10MB file made of /dev/urandom data)
> ##############a (10MB file made of /dev/urandom data)
> 
> Then, (as a normal user execute: "kioclient ~/src ~/dst")
> 
> Expected result: Notification that some files were not able to be copied (Cannot access "b" folder)
> Result with latest kdelibs: Silent copying where b folder is owned by user but nothing inside.
> ---When all kdebugdialog flags are set, a backtrace is also seen.
>       
> subError notifications for listjob subjobs are lost, Copyjob uses this signal to skip data.
> 
> Also, this fix prevents a race condition. 
> 
> ListJob has another ListJob as a subjob.
> 
> Chaild fails for whatever reason and calls error, which calls slotError, which calls slotsFinished and emitResult.
> The result of the child gets collected by parent. slotResult of parent gets called, removes subjob and emitsresult because there are no subjobs left.
> ---That's fine as long as the slave associated with the parent emits finished before the child fails. If it doesn't, parent calls emitsResult twice.
> 
> 
> Result after patch: kioclient shows a MessageBox telling the copy operation could not be completed.
> 
> 
> Thanks,
> 
> Alberto Jiménez Ruiz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20151019/5449823c/attachment.htm>