Skip to content

Fix crash while loading hunks#944

Draft
Murmele wants to merge 2 commits intomasterfrom
fix-crash
Draft

Fix crash while loading hunks#944
Murmele wants to merge 2 commits intomasterfrom
fix-crash

Conversation

@Murmele
Copy link
Owner

@Murmele Murmele commented Feb 23, 2026

Reason: When calling QApplication::processEvents() all events are processed also user inputs which might lead to change the diff while we are reading the hunks which leads to a clear of the files where the fetchMore is currently working on it which is not correct and leads to undefined behaviour

Reason: When calling QApplication::processEvents() all events are processed also user inputs which might lead to change the diff while we are reading the hunks which leads to a clear of the files where the fetchMore is currently working on it which is not correct and leads to undefined behaviour
@Murmele
Copy link
Owner Author

Murmele commented Feb 24, 2026

Seems not to fix it yet

mFiles.size() < mDiffTreeModel->fileCount(dtw->selectedIndex());
}

class Guard {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not observed this crash, but just to offer some thoughts based on the code changes. Presumably this involves two or more threads, which means you need to ensure data is synchronised between them (specifically synchronise L1 and L2 cache that might not be shared). The C++ way of solving that would be std::atomic_bool, which has load and store that will specify how to deal with the inter-thread synchronisation. There's more about this in https://en.cppreference.com/w/cpp/atomic/atomic.html and https://en.cppreference.com/w/cpp/atomic/memory_order.html

The second thing here is that I'd suggest not to define this class. I believe you can achieve the same effect by using std::mutex and instead of using a std::lock_guard or similar mechanisms, just use try_lock. If that fails, you can do mCancelFetchMore.store(false) (maybe with memory order specifically set), and post QApplication::processEvents you can unlock and check with mCancelFetchMore.load() (which also might need a memory order set)

Copy link
Owner Author

@Murmele Murmele Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought at the beginning as well that two or more threads are involved, but there aren't.
When I call QApplication::processEvents() the current loop will be interrupted and mouse events are handled. This mouse event triggeres then again a new diff (when clicking on another element in the commitlist) which triggers the fetch.
The problem is that you cannot wait until the previous fetch is finished because it will never, because everything runs in a single thread so we have to return to go on in the fetch loop.

This is reproducable when you have a large number of hunks in a diff which must be loaded.

The Guard was just there to release the lock after the function returns so it cannot be forgotten.

But this actually does not solve the problem yet so I have to further investigate

@Murmele Murmele marked this pull request as draft March 2, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants