Break recursive loop causing Dolphin to crash when linking 20000 objects from one directory to another.
Details
Details
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| src/core/copyjob.cpp | ||
|---|---|---|
| 814 | about @anthonyfieroni comment | |
| 892 | I guess we can update this comment | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 814 | It'll not help, message queue will be blocked and you kill the from other thread, which is not what we want. | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 809 | Why did you remove this call? Your patch has no context (please use arc to upload patches to phabricator), but I can see here that this is in the CopyJobPrivate::skipSrc method. Did you test skipping files? This isn't mentioned in your test plan. Does the unittest jobtest still pass? | |
| 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. | |
| 849 | This still recurses. How does this get rid of recursion? I don't get it. | |
| 892 | Why? it still recurses. | |
| 915 | This makes no sense to me. We are finished when the previous phase is actually finished. | |
Comment Actions
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 cause the finish the copy prematurely.
The tests currently don't pass, they just hang.
| src/core/copyjob.cpp | ||
|---|---|---|
| 809 | This is in statNextSrc() | |
| 849 | Remove comment | |
| src/core/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. | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 915 | Not if we just launched subjobs and we haven't gotten the signal that they are finished. | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 814 |
Yes, i'm afraid of loop that can block the event queue, !isKilled will not work in single thread environment. | |
| src/core/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 return; for now". | |
| src/core/copyjob.cpp | ||
|---|---|---|
| 814 | So if m_currentStatSrc does not point to m_srcList end it should add timer event to queue returning back here in next queue iteration. | |