Make Server thread-safe with per-thread sessions - #1871
Conversation
|
Thanks for the contribution! Before we can merge this, we need @dxdc to sign the Salesforce Inc. Contributor License Agreement. |
|
signed the cla |
|
Nice change. Now that I ran the tests, it looks like your tests managed to catch an actual bug :D test_auth_state_is_set_atomically fails in our test run on Python 3.14t (the free-threaded, no-GIL matrix job) — reading token and site outside a lock lets a writer swap between the two attribute reads, so a reader can see ('token-a', 'site-b'). Under the GIL the window's small enough that it's incidentally hidden; 3.14t exposes it. The cleanest fix is probably to bundle the auth state into a single tuple or dataclass and swap the reference atomically — reader properties then do one self._auth_state read into a local and destructure. I'm also going to close and re-open this PR because sometimes that's what it takes to kick the cla-bot. |
jacalata
left a comment
There was a problem hiding this comment.
needs test fix for 3.14t
Motivation
Audit result from #1148: all HTTP traffic flows through a single requests.Session stored on Server, and requests.Session is not guaranteed to be thread-safe (psf/requests#2766 - connection pool and cookie state can be corrupted under concurrent use). Auth state (_auth_token, _site_id, _user_id, _site_url) is also written field-by-field with no lock, so a concurrent reader could observe a token paired with the wrong site during switch_site or re-sign-in.
The practical consequence was that the common pattern of sharing one Server across a ThreadPoolExecutor (e.g. bulk workbook downloads) was unsafe, and the workaround was one fully signed-in Server per thread.
Behavior change
Server.sessionnow lazily returns a per-threadrequests.Sessioncreated fromsession_factory, cached inthreading.local. This is exactly the "one session per thread" guidance from the requests maintainers, applied transparently. Thread pool workers reuse their session (and its connection pool) across tasks._set_auth/_clear_authare guarded by a lock so the (site, user, token) triple is always updated atomically. Reads stay lock-free._clear_authbumps the epoch, and each thread lazily replaces its cached session on next use. In-flight requests on other threads are not disrupted, matching the old re-assignment semantics.Server.close()and context-manager support (with TSC.Server(...) as server:). Sessions created for any thread are tracked in aWeakSet(weak, so sessions of exited threads can still be garbage collected) andclose()closes them all, releasing pooled connections.close()is transport-level only - it does not sign out - so it composes with the existingauth.sign_in()context manager, which already handles sign-out. The Server remains usable afterclose(); the epoch bump means any later call creates fresh sessions instead of hitting closed pools.Serverdocstring: share one instance freely, sign in (and calluse_server_version()) before spawning workers, and note thatsession_factorymay now be called once per thread.No public API changes. All endpoint code already routed through the
sessionproperty, so the change is confined toserver.py. The one private-surface change is thatServer._sessionno longer exists as an attribute.New
test/test_thread_safety.py:test_each_thread_gets_its_own_session- N threads held live behind a barrier each see a distinct, per-thread-stable session objecttest_session_factory_called_once_per_thread- factory runs exactly once per thread despite repeatedsessionaccesstest_sign_out_invalidates_sessions_of_all_threads- afterauth.sign_out(), both the signing-out thread and a still-alive worker thread get fresh sessionstest_concurrent_api_calls_use_per_thread_sessions- 8-workerThreadPoolExecutormaking 40 mockedusers.get()calls; every request completes and carries the shared auth tokentest_auth_state_is_set_atomically- reader threads hammerauth_token/site_idwhile the main thread cycles_set_auth/_clear_auth; no reader ever observes a mismatched token/site pairtest_close_closes_sessions_of_all_threads-close()closes the sessions of the constructing thread and all workers, and the server remains usable afterwardstest_close_does_not_sign_out- auth token survivesclose()test_context_manager_closes_on_exit-__enter__returns the server,__exit__closes sessionsVerified the first two tests fail against the current
developmentbranch.