jsonrpc: stop the server loop and its worker threads on shutdown

[hecko]

The bug

Neither JSON-RPC thread can be stopped:

void JsonRPCServerLoop::on_stop() { INFO("todo\n"); }
void RpcServerThread::on_stop()   { INFO("TODO: stop server thread\n"); }

RpcServerThread::run() loops on while (true), the loop thread sits in ev_loop(), and nothing even asks them to stop: the module has no destructor and RpcServerThreadpool has no teardown.

AmPlugIn::~AmPlugIn() deletes every plug-in factory and then dlclose()s the modules. While that happens the JSON-RPC loop thread keeps accepting connections and dispatching events, and the worker threads keep processing requests: they execute code out of a mapping that is being unloaded and touch JsonRPCServerLoop's static connection map after the module is gone. Same shape as the reg_agent dialer thread fixed in #570.

The fix

Make both stoppable and stop them in order:

• RpcServerThread gets a stop flag honoured by run(); request_stop() sets it and then wakes a thread blocked in waitForEvent() via ev_pending (that ordering matters: the other way round the thread can wake, drain and block again).
• RpcServerThreadpool::cleanup() stops, joins and deletes all workers.
• JsonRPCServerLoop::request_stop() breaks ev_loop() through an async watcher that the loop thread registers itself; only ev_async_send() may be called from another thread while the loop runs. run() then cleans up the thread pool, by that point no event can reach a worker any more, and closes the listening socket, which was leaked on every error path too.
• A new JsonRPCServerModule destructor stops and joins the loop. It goes through the singleton deliberately: the factory object EXPORT_PLUGIN_CLASS_FACTORY hands to AmPlugIn (and which AmPlugIn deletes) is not the instance that owns server_loop, so server_loop is initialised in the constructor as well.

AmThread::stop() detaches the thread, which would turn a following join() into a no-op, hence request_stop() alongside on_stop() in both classes, the pattern already used for the reg_agent dialer.

Follow-ups from the review

The review found four gaps in the shutdown sequence. Each is fixed in its own commit:

• A stop requested before the loop was up got lost. request_stop() wrote to the async watcher that run() initialises right before ev_loop(), and ev_async_init() resets it. A shutdown right after the module was loaded therefore left the loop running and the destructor's join() hanging. request_stop() now sets a flag first, and run() checks it once the watcher is live; a request from before shows in the flag, a later one reaches the watcher. Leaving the loop also stops the async watcher that hands events to it, so no watcher of the module stays registered in the default loop. (ev_async_send() on a never-started watcher touches nothing invalid, by the way; the request was merely dropped.)
• cleanup() could deadlock. It joined the workers with threads_mut held. A worker finishing an event hands its connection back through returnConnection(), which dispatches a pending event for that connection to the pool, and dispatch() takes threads_mut. The threads are now moved out of the pool under the mutex and joined without it; dispatch() finds the pool empty meanwhile and drops the event.
• The startup worker outlived the module when loading failed early. The pool is a static and starts a thread as soon as the module is loaded, for other modules to use JSON-RPC while they initialise. If loading fails before onLoad(), _instance is still NULL and the destructor guard skipped everything. The pool's own destructor now runs cleanup(), and the pool is defined after the other statics of the file so that it is destroyed before the ones its threads use.
• Connections were left open. After the loop returned, accepted and outgoing connections stayed registered with open descriptors and read watchers in the default loop, and the events queued for busy connections stayed pending. run() now closes and frees them once the workers are joined. For that, ev_is_active() has to be reliable on a connection that never entered the read loop, so the connection constructor initialises its watchers.

One trade-off worth stating: shutdown now waits for the workers. A worker parked in netstringsBlockingWrite() against a peer that has stopped reading retries every 10 ms without limit and delays it, which is the same unbounded wait AmMediaProcessor::stop() already does for its threads. A bound on those retries would be a separate change.

Known failure, not yet fixed

The hardened and ubsan CI jobs fail at process exit:

core/log.cpp:193: runtime error: member call on misaligned address ... for type 'struct AmLoggingFacility'
    #0 run_log_hooks core/log.cpp:193
    #1 RpcServerThreadpool::cleanup() apps/jsonrpc/RpcServerThread.cpp:217
    #2 RpcServerThreadpool::~RpcServerThreadpool() apps/jsonrpc/RpcServerThread.cpp:184
    #3 exit

cleanup() logs through DBG(), and when it runs from the static destructor of the pool the logging facilities it reaches have already been torn down. Ordering the pool's definition after the other statics of its own translation unit does not cover facilities that live elsewhere. The static-destructor path needs to stop logging, or the pool needs to be torn down before exit rather than by a static destructor.

Tests

core/tests/test_jsonrpc.cpp. The server loop and thread sources are built into sems_tests when libev is found (all CI builds have it); JsonRPC.cpp stays out because it exports the same factory symbol as uac_auth, so the test defines the two settings the loop reads from it. The suite is empty in a build without libev.

• A server thread asked to stop leaves run(), both when it is waiting for events and when it was asked before it started.
• cleanup() stops and joins the threads of a pool within a bound; dispatching to the emptied pool drops the event, and a second cleanup() is harmless.
• The loop accepts a client on its port; after request_stop() the loop thread leaves run() and the client sees its connection closed.
• A stop requested right after start(), or before it, is not lost. Without the flag, the first of these hangs.

The cleanup() deadlock needs a worker to dispatch while being joined and is not reproduced deterministically here.

Credit

Backported from yeti-switch/sems, which stops the same two threads on shutdown: the RpcServerThread stop flag and RpcServerThreadpool::cleanup() come from 977d0091, breaking out of ev_loop() from f6ab75ec, and the stop-flag-before-ev_pending ordering from 3bd8616d. Thanks to the yeti-switch maintainers.

[Copilot]

🟡 Changes recommended

Unresolved shutdown races, connection cleanup gaps, and a worker-pool deadlock block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds orderly shutdown for the JSON-RPC event loop and worker threads before module unloading.

Changes:

• Adds worker stop signaling and pool cleanup.
• Adds async event-loop shutdown and listening-socket cleanup.
• Stops and joins the loop during module destruction.

File summaries

File	Reviewed change
apps/jsonrpc/RpcServerThread.h	Declares worker stop and cleanup APIs.
apps/jsonrpc/RpcServerThread.cpp	Implements worker shutdown and joining.
apps/jsonrpc/RpcServerLoop.h	Declares loop stop support.
apps/jsonrpc/RpcServerLoop.cpp	Implements async loop shutdown and socket cleanup.
apps/jsonrpc/JsonRPC.cpp	Stops and joins the loop during destruction.

Review details

Suppressed comments (1)

apps/jsonrpc/RpcServerLoop.cpp:193

• async_w is never stopped when the loop exits. It remains registered in the default libev loop with async_cb pointing into this module, so unloading the module leaves a live watcher referencing unmapped code; a later reuse of the default loop (for example during a reload) can call it. Stop async_w in this callback before breaking the loop.

  ev_async_stop(EV_A_ w);
  ev_io_stop(EV_A_ &ev_accept);
  ev_break(EV_A_ EVBREAK_ALL);

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.