From: Mark Jamsek Subject: tog: fix race condition between main and log thread To: gameoftrees@openbsd.org Date: Mon, 30 Dec 2024 23:38:53 +1100 This got lost on IRC the other day when I should have sent it to the list: stsp reported that the tog test_log_mark_keymap regression test would randomly hang, which I was able to reproduce. This is due to a race condition between the main thread checking the log_complete flag and the log thread setting it. When the F keymap is entered to toggle fullscreen, the main thread might need more commits queued to fully populate the log view when redrawing the screen. So it first checks the log_complete flag to see if the commit graph has already been traversed and, if so, doesn't bother waking the log thread to request any more commits. But if the log_complete flag isn't set, it does wake the log thread and than waits for it to signal when a commit is loaded. In the log thread, after we have finished traversing the commit graph, we briefly release the mutex before setting the log_complete flag and this causes the race: the main thread checks and the flag is unset so it wakes the log thread and then waits on the log thread in trigger_log_thread() to signal when a new commit is loaded. In between the main thread checking the flag, the log thread exits the loop and sets the flag. The fix sets the flag in the loop before releasing the mutex. And because we can exit the loop on error or receipt of a fatal signal, we also keep the existing log_complete assignment outside the loop. commit 89dc2ce648b3694fe5f8a48991bb0edfd5901360 (main) from: Mark Jamsek date: Fri Dec 27 12:24:27 2024 UTC tog: fix log view race condition evinced by regress When the commit graph has been traversed, we set the log_complete flag to signal the main thread that we've completed iterating commit history, which request_log_commits() checks and, if set, returns early instead of calling trigger_log_thread(). If called, trigger_log_thread() wakes the log thread, then waits on the log thread to unblock when another commit has been loaded. There's a race between the log thread setting the log_complete flag and request_log_commits() checking it, which causes trigger_log_thread() to wait on a signal that never unblocks because there are no more commits to be loaded. M tog/tog.c | 2+ 0- 1 file changed, 2 insertions(+), 0 deletions(-) commit - 6c185310f28c4ac8a4ed01a7e6f2991529017b28 commit + 89dc2ce648b3694fe5f8a48991bb0edfd5901360 blob - b37bfa5a5a29303875829cda627929d35d603f5f blob + fc81e400b4e1984a83056f6313f5a785f103e6a1 --- tog/tog.c +++ tog/tog.c @@ -4003,6 +4003,8 @@ log_thread(void *arg) TAILQ_FIRST(&a->real_commits->head); *a->selected_entry = *a->first_displayed_entry; } + if (done) + a->log_complete = 1; errcode = pthread_cond_signal(&a->commit_loaded); if (errcode) { -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68