D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:814 > Right. If this loop can really block the main thread for a substantial amount > of time, then it should have a condition like "after 100 symlinks, schedule > coming back here (e.g. QTimer singleshot) and `

D28669: make CopyJob non-recursive

dfaure added inline comments. INLINE COMMENTS > anthonyfieroni wrote in copyjob.cpp:814 > > Which threads? There are no threads involved here. > > Yes, i'm afraid of loop that can block the event queue, !isKilled will not > work in single thread environment. Right. If this loop can really bloc

D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:814 > Which threads? There are no threads involved here. > > There is no need to handling killing here. It wasn't any different in the > orig code with the recursion. > As soon as we find actual work to do, we'l

D28669: make CopyJob non-recursive

dfaure added inline comments. INLINE COMMENTS > meven wrote in copyjob.cpp:915 > When reaching here the `while (m_currentStatSrc != m_srcList.constEnd()) {` > means we have nothing left to stat, meaning stating phase is indeed finished. Not if we just launched subjobs and we haven't gotten the

D28669: make CopyJob non-recursive

meven added inline comments. INLINE COMMENTS > dfaure wrote in copyjob.cpp:915 > This makes no sense to me. We are finished when the previous phase is > actually finished. When reaching here the `while (m_currentStatSrc != m_srcList.constEnd()) {` means we have nothing left to stat, meaning st

D28669: make CopyJob non-recursive

meven added a comment. Currently the code does not handle the other call paths to `statNextSrc()` that would need to be adapted as well. We will need to call `statCurrentSrc()` after `statNextSrc()` basically there. In slotResultRenaming and slotResult. Currently, those code path would ca

D28669: make CopyJob non-recursive

dfaure requested changes to this revision. dfaure added inline comments. INLINE COMMENTS > copyjob.cpp:809 > ++m_currentStatSrc; > -statCurrentSrc(); > } Why did you remove this call? Your patch has no context (please use `arc` to upload patches to phabricator), but I can see here th

D28669: make CopyJob non-recursive

anthonyfieroni added inline comments. INLINE COMMENTS > meven wrote in copyjob.cpp:814 > about @anthonyfieroni comment > Add `&& !isKilled()` with a code path to handle it properly. It'll not help, message queue will be blocked and you kill the from other thread, which is not what we want. REP

D28669: make CopyJob non-recursive

meven requested changes to this revision. meven added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > copyjob.cpp:814 > Q_Q(CopyJob); > -if (m_currentStatSrc != m_srcList.constEnd()) { > +while (m_currentStatSrc != m_srcList.constEnd()) { >

D28669: make CopyJob non-recursive

anthonyfieroni added a comment. If you want to kill the job how this loop will be break? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28669 To: McPain, #frameworks, dfaure, meven, ahmadsamir Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh,

D28669: make CopyJob non-recursive

kossebau edited reviewers, added: Frameworks, dfaure, meven, ahmadsamir; removed: kossebau. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28669 To: McPain, #frameworks, dfaure, meven, ahmadsamir Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28669: make CopyJob non-recursive

McPain created this revision. McPain added a reviewer: kossebau. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. McPain requested review of this revision. REVISION SUMMARY Break recursive loop causing Dolphin to crash when linking 2 objects from one dire