cleanupDocument may delete complete document

[letsfindaway]

I had to disable the cleanupDocument feature. Every scenario where you insert new pages on a document already containing some produce the bug where document is immediately removed (in our context only) !

Originally posted by @kaamui in https://github.com/letsfindaway/OpenBoard/issues/162#issuecomment-1923605001

[letsfindaway]

I think the problem is that cleanupDocument() scans the document directory for pages, but there might be unsaved pages due to the asynchronous thread for saving. This occurs especially with network drives, where the drive access needs longer. So the scanning for UUIDs is missing some pages and therefore cleaning referenced objects.

This is a general problem of the current code. As long as any work has to be done by the worker thread such scanning is not save.

Solutions could be:

• Don't clean when there is still work to be done in the persistence worker queue, i.e. mScenesToSave is not empty.
• Get the UUIDs from scanning the scenes, not the page files. But can we be sure that all pages are loaded to scenes?
• Only do the cleanup when a document is closed and then after all pages are saved.

As there is no real "closing" of a document and also scanning the scenes is not an option as they could also be incomplete, we will probably end with the first approach. What about calling the cleanup here: https://github.com/letsfindaway/OpenBoard/blob/e6d2ba36c28dd49d848cb72cf387f1f91cc7aed3/src/core/UBPersistenceManager.cpp#L146-L150

as follows:

    if (mScenesToSave.isempty())
    {
        cleanupDocument(scene->document());
    }

Be careful!

• This function gets the copy of the scene. Can we get the proxy from there? Yes, we can. The proxy pointer is copied when the scene is copied.
• As soon as the shared pointer is deleted from the set, the object is deleted! So it is no longer available after that. We have to get the proxy before removing the scene.

So the final code could be:

void UBPersistenceManager::onScenePersisted(UBGraphicsScene *scene)
{
    // remember the document proxy
    auto documentProxy = scene->document();

    // delete the copy
    mScenesToSave.removeAll(scene->shared_from_this());

    // cleanup if no more scenes are waiting
    if (mScenesToSave.isEmpty())
    {
        cleanupDocument(documentProxy);
    }
}

[letsfindaway]

In addition to this we should check the code for other occurrences of accessing the document files. The asynchronous behavior of loading and saving through workers may also affect those. Way too many places use UBDocumentProxy::persistencePath().

[letsfindaway]

@kaamui: do you remember other cases where the network storage was causing problems (or perhaps better making existing problems obvious)?

[kaamui]

I think the problem is that cleanupDocument() scans the document directory for pages, but there might be unsaved pages due to the asynchronous thread for saving.

No. Our problem was a completely different one. Due to a parameter cache=loose set for performance reasons, when cleanupDocuments scans the document directory for pages, the Qt equivalent for ls returns an outdated answer. The pages are on the disk, but the smb client cache is not aware of the new pages at the time OpenBoard is scanning the directory again.

[letsfindaway]

No. Our problem was a completely different one. Due to a parameter cache=loose set for performance reasons, when cleanupDocuments scans the document directory for pages, the Qt equivalent for ls returns an outdated answer. The pages are on the disk, but the smb client cache is not aware of the new pages at the time OpenBoard is scanning the directory again.

Thanks for clarifying.

Solutions could be:

• Don't clean when there is still work to be done in the persistence worker queue, i.e. mScenesToSave is not empty.
• Get the UUIDs from scanning the scenes, not the page files. But can we be sure that all pages are loaded to scenes?
• Only do the cleanup when a document is closed and then after all pages are saved.

This means that the second approach is the only way, as it does not rely on any information from the file system. We would have to disable scanning and cleanup while not all pages are loaded.

To scan a scene, we would simply iterate over all items and call uuid() on each item which can be converted to a UBItem. No more regular expressions.

[letsfindaway]

We should however also be careful as scanning may take a long time in a network scenario. To complete a scan, all scene files have to be read, and this currently happens in the main thread. So any attempt to improve performance by caching adjacent scenes is thwarted. See also #171.

However ALL scenes are loaded anyway for the thumbnails in UBBoardThumbnailsView::initThumbnails: https://github.com/letsfindaway/OpenBoard/blob/0e69a0a03248cbb2a5914fdbbc04772869c5836a/src/gui/UBBoardThumbnailsView.cpp#L153-L167

And this also happens on the main thread. So any attempt to improve this by using a worker thread is unnecessary.