Fix pollsession turn queue starving session addition and removal - #637
Merged
Conversation
michalvasko
requested changes
Sep 4, 2026
Roytak
force-pushed
the
pspoll-prio-queue
branch
7 times, most recently
from
September 10, 2026 08:08
a9cdfec to
e5de4c0
Compare
michalvasko
requested changes
Sep 10, 2026
michalvasko
left a comment
michalvasko
left a comment
Member
There was a problem hiding this comment.
Changes themselves look good, just an additional change seems appropriate.
The pollsession turn queue was strictly FIFO, so nc_ps_add_session() and the other bounded operations queued up behind every poll thread and each of them kept its turn for the whole timeout it was given. A newly established session was therefore not polled for up to thread_count * poll_timeout msec. Threads now queue up with a priority. Every operation except nc_ps_poll() only walks the session array, so it is queued up in front of all the poll threads, and the polling thread hands the turn over. For that to hold, the turn must never be held across a blocking wait. nc_ps_clear() now only collects the sessions under the turn and frees them after giving it up, because nc_session_free() waits for the session RPC lock, and the poll loop no longer waits for the session IO lock at all - it only does a non-blocking readiness check there, which a concurrent nc_server_notif_send() could otherwise block for an unbounded time. NC_PS_TIMEOUT is therefore lowered to 500 msec, as reaching it now really does mean the queue is jammed. The queue is also allocated and grows on demand instead of failing once MAX_PSPOLL_THREAD_COUNT threads use the same pollsession, which silently dropped the session being added. Waiting longer than NC_PS_TIMEOUT is only an error for the high priority threads now, a poll thread may legitimately wait behind several other poll threads for longer than that. Also fixes the session idle timeout being compared against the poll deadline instead of the current time, which terminated active sessions when the poll timeout was longer than the remaining idle time, and read an uninitialized timespec with an infinite timeout. Fixes #561
nc_ps_add_session() incremented session_count before reallocating the session array. nc_realloc() frees the original block when it fails, so on OOM the pollsession was left with a NULL array and a non-zero count. The next nc_ps_poll() passed its empty-pollsession check and dereferenced the NULL array, and every session added so far was lost. Reallocate into a temporary and publish the array and the count only once the session is actually stored, so a failure leaves the pollsession exactly as it was. Use plain realloc(), nc_realloc() is the free-on-failure variant and is wrong here.
nc_ps_poll() kept a terminated session in the pollsession and flagged its wrapper as invalid so that it was not reported twice. It wrote that flag after having given up the turn, while holding only the session RPC lock, but the wrapper is owned by the pollsession and any thread with the turn could free it in the meantime. nc_ps_del_session() does so right away, which made the write a use-after-free. It is not even a race when the RPC callback deletes its own session, that fails every time. The flag only ever carried "this session is dead and was reported already", which is only needed because the dead session stays in the pollsession. So remove it there instead, at the point its death is detected, and hand it over to the caller. The property is then structural rather than a flag, and struct nc_ps_session has nothing left in it, so ps->sessions becomes a plain session array. Termination is also detected during the RPC, when the turn is no longer held, so it is taken back for the removal. Only the thread whose own removal succeeded owns the session, the rest drop the termination bits and let a later poll report it. Since the session is no longer in the pollsession when it is reported, NC_PSPOLL_SESSION_TERM now transfers its ownership and the caller must free it. nc_ps_del_session() on it returns -1 and nc_ps_clear() will not find it, so the previously documented pattern of clearing the invalid sessions after a poll leaks them. Also fixes nc_ps_poll() remembering the index of a removed session as the last event one, which made the next poll start past the end of the session array if the terminated session happened to be the last.
Roytak
force-pushed
the
pspoll-prio-queue
branch
from
September 11, 2026 11:12
e5de4c0 to
0496c5d
Compare
Roytak
force-pushed
the
pspoll-prio-queue
branch
from
September 11, 2026 11:50
0496c5d to
5a16aee
Compare
michalvasko
approved these changes
Sep 11, 2026
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
2 participants
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.
The turn queue was strictly FIFO, so
nc_ps_add_session()and the otherbounded operations queued up behind every poll thread, each holding its turn
for the whole timeout it was given so a new session went unpolled for up to
thread_count * poll_timeoutmsec.Threads now queue up with a priority: everything except
nc_ps_poll()onlywalks the session array, so it is queued in front of the poll threads, and the
polling thread hands the turn over once it is no longer first. Queue entries
are
pthread_t, so there are no generated IDs. The queue also grows on demandinstead of failing once
PSPOLL_THREAD_COUNTthreads share a pollsession,which silently dropped the session being added.
Also fixes the session idle timeout being compared against the poll deadline
instead of the current time. New
test_ps_pollcovers the above.Fixes #561