Never block on the per-issuer fetch lock in the async path#216
Open
djw8605 wants to merge 2 commits into
Open
Conversation
The synchronous verify() loop only called verify_async_continue() when select() reported I/O activity (select_result > 0). select() clears the fd_sets on timeout, and the sets are only repopulated inside curl_multi_fdset() via verify_async_continue(), so after the first 50ms period with no socket activity the loop kept selecting on empty fd_sets and never drove libcurl again, spinning until expiry_time and failing with 'Timeout when loading the OIDC metadata'. libcurl also requires curl_multi_perform() to be called after a select() timeout so it can run its internal (non-socket) work such as DNS retries and connection timeouts. Continue on select_result >= 0 so timeouts drive the transfer forward. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_public_key_pem() acquired the per-issuer thundering-herd mutex (added in scitokens#180) with a blocking std::unique_lock and parked the held lock inside the returned AsyncStatus until the fetch completed. The async API exists so an event loop can interleave operations on one thread; if that thread called scitoken_deserialize_start for a second token from the same still-fetching issuer, it blocked forever on a non-recursive mutex held by its own earlier status -- a self-deadlock. Even across threads, a supposedly non-blocking _start call blocked for the full duration of another thread's fetch. Acquire the mutex with try_to_lock instead. On failure, return a status in a new 'waiting' state that, on each continue call, polls the keycache DB for the keys the lock holder is fetching (a negative-cache entry from a failed fetch propagates as before) and retries the lock in case the holder failed or was abandoned without storing anything. get_timeout_val() suggests a 100ms poll interval while waiting. Thundering-herd protection is preserved: concurrent operations for the same new issuer still result in a single web fetch, with the waiters picking the keys up from the keycache. Co-Authored-By: Claude Fable 5 <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
1 participant
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.
Problem
For a new issuer,
get_public_key_pem()acquires the per-issuer thundering-herd mutex (#180) with a blockingstd::unique_lockand parks the held lock inside the returnedAsyncStatusuntil the fetch completes.The async API exists so an event loop can interleave several operations on one thread. If that thread calls
scitoken_deserialize_startfor a second token from the same still-fetching issuer, it blocks forever on a non-recursive mutex held by its own earlier status — a self-deadlock. Even across threads, a supposedly non-blocking_startcall blocks for the full duration of another thread's web fetch.Fix
try_to_lock.m_waiting_for_issuer). Eachcontinuecall then:get_timeout_val()suggests a 100 ms poll interval while waiting, so both the sync select() loop and C-API event loops wake up promptly.Thundering-herd protection is preserved: concurrent operations for the same new issuer still produce a single web fetch, with waiters picking the keys up from the keycache.
Note: stacked on #211 (contains its commit) — the sync
verify()loop only re-polls on select timeout with that fix in place.Testing
ctestunit, env_config, and monitoring suites pass (includes the async deserialize tests).🤖 Generated with Claude Code