Follow-up to review feedback on #1636 (raised by CodeRabbit, endorsed by @mlubin: "this seems like a real issue").
Problem
cuOptSetLogCallback stores a callback per settings object, but it reaches the
logger through a single process-global slot in cpp/src/utilities/logger.cpp:
static log_callback_with_data_t g_pending_callback = nullptr;
static void* g_pending_callback_data = nullptr;
Flow: cuOptSolve drops the settings' callback into these globals via
set_pending_log_callback(), and init_logger_t picks it up, copies it into the
logger guard, and installs a callback_sink_mt.
Two failure modes follow, both only with concurrent or overlapping solves.
1. Clobbering. The mutex makes each write atomic, but not the
drop-then-consume pair. Thread A calls set_pending_log_callback(cbA), thread B
calls set_pending_log_callback(cbB) before A's init_logger_t runs, and A
silently gets B's callback.
2. Silent drop, with a stale pointer left behind. init_logger_t returns
early when a guard is already alive:
auto existing_guard = g_active_guard.lock();
if (existing_guard) { guard_ = existing_guard; return; }
So if solve A is running, solve B's init_logger_t never reads the mailbox: B's
callback is never installed, and B's registration stays in the globals. A later
solve can then pick it up and invoke B's function pointer with B's user_data,
which may already have been freed by then.
The second is the more serious one — it is not "some log lines go missing" but a
stale callback plus stale user data surviving the solve that registered them.
Root cause
default_logger() is a process-wide singleton with one global sink list, so
there is no per-solve context for user_log_bridge to route on. Per-solve
callback delivery needs solve identity threaded through the logging path.
Options
- Fail loudly. If a solve begins while a guard is active with a different
callback, return an error from cuOptSolve rather than silently dropping it.
Small and honest; rejects a legitimate use case.
- Thread-local registration. The bridge looks up a
thread_local callback,
so each solving thread gets its own. Covers lines emitted on the solve thread;
lines from internal worker threads would reach no callback.
- Per-solve logger context. Thread solve identity through the logging path
so the bridge can route correctly. Correct, and the largest change.
Whichever is chosen, clearing the pending slot on the early-return path would
remove the stale-pointer hazard on its own, and is worth doing regardless.
Scope
Single-solve use — the common case — is correct today, which is why #1636 ships
with this documented as a known limitation rather than blocked on it.
Follow-up to review feedback on #1636 (raised by CodeRabbit, endorsed by @mlubin: "this seems like a real issue").
Problem
cuOptSetLogCallbackstores a callback per settings object, but it reaches thelogger through a single process-global slot in
cpp/src/utilities/logger.cpp:Flow:
cuOptSolvedrops the settings' callback into these globals viaset_pending_log_callback(), andinit_logger_tpicks it up, copies it into thelogger guard, and installs a
callback_sink_mt.Two failure modes follow, both only with concurrent or overlapping solves.
1. Clobbering. The mutex makes each write atomic, but not the
drop-then-consume pair. Thread A calls
set_pending_log_callback(cbA), thread Bcalls
set_pending_log_callback(cbB)before A'sinit_logger_truns, and Asilently gets B's callback.
2. Silent drop, with a stale pointer left behind.
init_logger_treturnsearly when a guard is already alive:
So if solve A is running, solve B's
init_logger_tnever reads the mailbox: B'scallback is never installed, and B's registration stays in the globals. A later
solve can then pick it up and invoke B's function pointer with B's
user_data,which may already have been freed by then.
The second is the more serious one — it is not "some log lines go missing" but a
stale callback plus stale user data surviving the solve that registered them.
Root cause
default_logger()is a process-wide singleton with one global sink list, sothere is no per-solve context for
user_log_bridgeto route on. Per-solvecallback delivery needs solve identity threaded through the logging path.
Options
callback, return an error from
cuOptSolverather than silently dropping it.Small and honest; rejects a legitimate use case.
thread_localcallback,so each solving thread gets its own. Covers lines emitted on the solve thread;
lines from internal worker threads would reach no callback.
so the bridge can route correctly. Correct, and the largest change.
Whichever is chosen, clearing the pending slot on the early-return path would
remove the stale-pointer hazard on its own, and is worth doing regardless.
Scope
Single-solve use — the common case — is correct today, which is why #1636 ships
with this documented as a known limitation rather than blocked on it.