Stop per-peer streamer threads at shutdown to avoid teardown crash#329
Open
jolavillette wants to merge 1 commit into
Open
Stop per-peer streamer threads at shutdown to avoid teardown crash#329jolavillette wants to merge 1 commit into
jolavillette wants to merge 1 commit into
Conversation
At shutdown RsServer::rsGlobalShutDown() stopped the registered service
threads and the RsServer tick thread, but never the per-peer network I/O
threads (pqithreadstreamer). Those detached RsTickingThreads kept running
handleincoming() -> RsSerialiser::deserialise() -> new RsItem while the
process tore down its static objects, so SmallObject::operator new locked
the already-destroyed SmallObject::_mtx (pthread_mutex_lock -> EINVAL /
error 22) and hit the deactivated SmallObject::_allocator ("(EE) allocating
N bytes ... cannot be deleted"), flooding the log and risking a crash on
exit.
Add pqipersongrp::fullstopAllThreads(), which stops every peer's connection
threads (same as removePeer() but without deleting the persons), and call it
from rsGlobalShutDown() right after fullstop() so the RsServer tick thread is
no longer iterating the peer list concurrently.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stop per-peer streamer threads at shutdown to avoid teardown crash
Fix: crash / hang when closing RetroShare (shutdown thread-teardown race)
Symptom
When closing RetroShare, the log would sometimes show this pair of lines, often
repeated in bursts, and the process would fail to exit cleanly (in the worst
case it had to be killed):
Both messages actually originate from the same line,
SmallObject::operator newin
libretroshare/src/util/smallobject.cc:Root cause
RsServer::rsGlobalShutDown()(libretroshare/src/rsserver/p3face-config.cc)stopped the registered service threads and the RsServer tick thread, but never
the per-peer network I/O threads (
pqithreadstreamer, one per connection).These are detached
RsTickingThreads that keep looping:Nothing stopped them at shutdown (the only path that does,
removePeer(), onlyruns when a friend is removed), so they outlived the whole shutdown sequence.
When the process finally exited, the static destructors in
smallobject.ccran:
~SmallObjectAllocatorsets_active = false;~RsMutexcallspthread_mutex_destroy(&_mtx).Meanwhile those still-running streamer threads were deserialising an incoming
packet ->
SmallObject::operator new->RsStackMutex(_mtx)on thealready-destroyed mutex ->
pthread_mutex_lockreturnsEINVAL(error 22),and
_active == falsetriggers themallocfallback plus the "cannot bedeleted" message.
Net effect: concurrent access to static objects being torn down = undefined
behaviour, a flood of log lines, and a process that would not terminate
cleanly. Databases were already closed before this point, so there was no data
loss, but shutdown was broken.
This is a Linux destruction-ordering race, distinct from the Windows/Qt6
emutlsthread-exit issue.Fix
Add
pqipersongrp::fullstopAllThreads(), called fromrsGlobalShutDown()rightafter
fullstop()(so the RsServer tick thread is no longer iterating the peerlist concurrently), to stop all per-peer streamer threads before the static
objects are destroyed.
Two subtle pitfalls, both diagnosed live with gdb, had to be handled:
a) Deadlock. A first version held
coreMtxwhile waiting for the threads.But a streamer thread that is delivering a received item needs
coreMtxto sendits reply (
pqihandler::queueOutRsItem). Cycle:main -> holds coreMtx -> waits for streamers;p3rtt thread -> holds the rtt service mutex -> waits for coreMtx;~50 streamers -> wait for the rtt mutex. Permanent hang.=> Two phases: signal stop + close sockets while holding
coreMtx(non-blocking), release
coreMtx, then wait for the threads to stop.b) SIGSEGV. A blind
(pqiperson*)it->second->pqicast over the wholemodsmap. But
modsalso contains thepqiloopback(the node's own loopback).pqiloopbackis a sibling ofpqiperson(both derive fromPQInterface),not a
pqiperson, so the cast reinterpreted unrelated memory -> garbagemPersonMtx(error 22) then crash.removePeer()never hit this because itlooks up a specific friend id, never the loopback.
=> Use
dynamic_cast<pqiperson*>and skip non-persons (the loopback has nonetwork threads to stop anyway).
Final implementation (
libretroshare/src/pqi/pqipersongrp.cc):And the call site in
RsServer::rsGlobalShutDown():Result
At shutdown the streamer threads are now stopped before static teardown: no more
error 22/ cannot be deleted flood, no deadlock, no SIGSEGV, clean exit.