Initialize logging queues before workers - #13509
Conversation
Pre-initialization plugin log buffers can be waiting when the logging workers start. A preprocessing thread can consume one before the flush queue exists and crash traffic_server while pushing the buffer to a null queue. This patch addresses the initialization race by constructing every logging notification and queue before spawning either worker. No logging thread can observe partially initialized shared queue state. This completes the startup ordering protection from apache#13472, which prevents plugins from waking a preprocessor before its notification exists but does not protect the flush queue after that worker starts.
JosiahWI
left a comment
JosiahWI
left a comment
There was a problem hiding this comment.
The ink_atomiclist_init is moved before the spawn_thread loop to establish a happens-before relationship.
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
The fix is correct and the diagnosis checks out.
The crash path is real and reachable exactly as described:
create_threads()spawns the preproc threads atLog.cc:1252, butflush_data_listwas allocated atLog.cc:1263— after.- A preproc thread that finds a pre-init plugin buffer already queued reaches
LogFile::preproc_and_try_delete()→ink_atomiclist_push(Log::flush_data_list, flush_data)(src/proxy/logging/LogFile.cc:473, again at:649) andLog::flush_notify->signal()(:475,:651) — both on null pointers.
After the move, every consumer of both globals is initialized before any thread that can touch them exists, and eventProcessor.spawn_thread() supplies the happens-before edge for the plain pointer writes, so no atomics or barriers are needed here.
I also checked the other consumer, PeriodicWakeup, which dereferences preproc_notify[i] and flush_notify[i]: it is scheduled at Log.cc:1216, after create_threads() returns, so there is no second gap. The claim that no logging thread can observe partially initialized queue state holds.
Suggestions (non-blocking)
- Leave a note at the new location. The whole point of the change is a non-obvious ordering constraint, and the code now reads as an unexplained hoist above an unrelated
stacksizefetch. One line — "must be initialized before any preproc thread starts; preproc →LogFile::preproc_and_try_delete()pushes toflush_data_list" — will stop someone tidying it back next year. The// Now, only one flush thread is supported / TODOcomment left behind now sits above only the continuation spawn, which is still accurate. - Consider a cheap invariant at the
LogFile.cc:473/:649push sites (ink_release_assert(Log::flush_data_list != nullptr)). It converts any future recurrence from a null deref in a worker thread into an obvious assert with a usable stack. - No test is understandable for a startup-ordering race — worth saying so explicitly in the description, along with how this was found. A linked stack trace would make this trivially reviewable for a backport.
Drive-by, not this PR
PeriodicWakeup::wakeup() indexes Log::flush_notify[i] (Log.cc:189) although flush_notify is a single new EventNotify, not an array. Safe today only because the constructor is always called with flush_threads == 1 (Log.cc:1216). If anyone acts on the multiple-flush-threads TODO, that is an out-of-bounds read waiting to happen.
Pre-initialization plugin log buffers can be waiting when the logging workers start. A preprocessing thread can consume one before the flush queue exists and crash traffic_server while pushing the buffer to a null queue. This patch addresses the initialization race by constructing every logging notification and queue before spawning either worker. No logging thread can observe partially initialized shared queue state. This completes the startup ordering protection from #13472, which prevents plugins from waking a preprocessor before its notification exists but does not protect the flush queue after that worker starts. (cherry picked from commit 3177997)
|
Cherry-picked to the 10.2.x branch as 5e0c7e9 for the 10.2.0 release. |
Pre-initialization plugin log buffers can be waiting when the logging
workers start. A preprocessing thread can consume one before the flush
queue exists and crash traffic_server while pushing the buffer to a null
queue.
This patch addresses the initialization race by constructing every
logging notification and queue before spawning either worker. No logging
thread can observe partially initialized shared queue state.
This completes the startup ordering protection from #13472, which
prevents plugins from waking a preprocessor before its notification
exists but does not protect the flush queue after that worker starts.