D23721: Fix race condition in TextBuffer
thomassc abandoned this revision. thomassc added a comment. Okay, thanks for your feedback. Discarding this then. I think I might have found the problematic access in KDevelop already in the meantime ... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D23721 To: thomassc, #ktexteditor Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, sars
D23721: Fix race condition in TextBuffer
dhaumann added a comment. KTextEditor is nowhere designed to be thread-safe. Even if this patch solves a particular issue you have in multithreaded access, we can be sure there will be other (corner?) cases where it won't work: Think only of modifying the text at some other place at the same time. It simply does not work right now. That said: Please always access KTextEditor API only from the main thread, and none else. -1 from my side. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D23721 To: thomassc, #ktexteditor Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, sars
D23721: Fix race condition in TextBuffer
thomassc added a comment. Okay, I wasn't aware of the overall situation wrt. threading. Still, couldn't it be useful to at least allow for concurrent read accesses which is probably not a large effort (in contrast to concurrent read and write)? I guess that people might not expect blockForLine() to not be re-entrant, and it seems like the proposed robustification might not add too much overhead for performance and code complexity. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D23721 To: thomassc, #ktexteditor Cc: cullmann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann
D23721: Fix race condition in TextBuffer
cullmann added a comment. Hmm, the use of atomic + a local var might avoid that the compiler optimizes away the local var and just uses m_lastUsedBlock again directly, but I am not sure if we should go that way. If one has concurrent accesses to the buffer from different threads, all is lost, as no part of the buffer is really thread save. (e.g. it will still random crash if the buffer is modified in the main thread). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D23721 To: thomassc, #ktexteditor Cc: cullmann, kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, sars, dhaumann
D23721: Fix race condition in TextBuffer
thomassc edited the summary of this revision. thomassc added a reviewer: KTextEditor. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D23721 To: thomassc, #ktexteditor Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
D23721: Fix race condition in TextBuffer
thomassc created this revision. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. thomassc requested review of this revision. REVISION SUMMARY In the "shortcut: try last block first" branch in TextBuffer::blockForLine(), m_lastUsedBlock is accessed multiple times. If another tread simultaneously executes the binary block search below in the same function, then m_lastUsedBlock may be modified by that thread. This may result in the first thread returning a wrong block, since it may return the modified value of m_lastUsedBlock instead of the original value that it used to determine whether the given line is in the block. This change is intended to fix a crash where an out-of-bounds line is accessed in katetextblock.cpp, which occurs about one or two times per month for me when using KDevelop. I have no idea whether this possible race condition is actually the reason for these crashes, but it seems like a plausible candidate. TEST PLAN I only tested that the code still works after the change. REPOSITORY R39 KTextEditor BRANCH fix_katetextbuffer_race_condition REVISION DETAIL https://phabricator.kde.org/D23721 AFFECTED FILES src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h To: thomassc Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, GB_2, domson, michaelh, ngraham, bruns, demsking, cullmann, sars, dhaumann
