Skip to content

Fix data race in connection pool by running QuicConnStart on the connection's worker thread#6114

Merged
guhetier merged 9 commits into
mainfrom
fix-connection-pool-start-race
Jun 30, 2026
Merged

Fix data race in connection pool by running QuicConnStart on the connection's worker thread#6114
guhetier merged 9 commits into
mainfrom
fix-connection-pool-start-race

Conversation

@gaurav2699

@gaurav2699 gaurav2699 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Description

QuicConnPoolTryCreateConnection called QuicConnStart directly on the application thread instead of queuing it to the connection's worker.
Partway through, QuicConnStart calls QuicBindingAddSourceConnectionID, which publishes the connection's source CID into the binding lookup table.
From that point the receive datapath can match inbound packets to the connection and queue work to its worker. Because the start was running outside the per-connection operation queue, the OperQ->ActivelyProcessing gate was never set, so the first inbound packet caused the worker to begin draining the same connection in parallel with the app thread still inside QuicConnStart.
That breaks msquic's single-threaded-per-connection invariant: two threads then touch Connection->Crypto concurrently.
Fixes #6058

Testing

The spinquic stress test exercises this path

Documentation

NA

@gaurav2699
gaurav2699 requested a review from a team as a code owner June 22, 2026 13:13
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.00%. Comparing base (f39550f) to head (1217e79).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/core/connection.c 50.00% 1 Missing ⚠️
src/core/connection_pool.c 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6114      +/-   ##
==========================================
- Coverage   86.13%   85.00%   -1.13%     
==========================================
  Files          60       60              
  Lines       18847    18911      +64     
==========================================
- Hits        16233    16076     -157     
- Misses       2614     2835     +221     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@guhetier guhetier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable (although I have performance concerns if we need to queue every connection start this way, we will need to validate that).
The CI failures are likely pointing to some remaining issue though.

Comment thread src/core/operation.h Outdated
Comment thread src/core/connection_pool.c Outdated
Comment thread src/core/connection_pool.c Outdated
Comment thread src/core/connection.c Outdated
Comment thread src/core/connection_pool.c Outdated
Comment thread src/core/connection_pool.c Outdated
@anrossi

anrossi commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

I agree with Guillaume's feedback. My general comments on the whole PR would be that the connection pools assumed they had ownership/control of connection start on the caller thread, so keep an eye out for corner cases built on that assumption.
Same goes for the ExternalOwner flag, but I think you are fixing that up.

Comment thread src/core/operation.h
guhetier
guhetier previously approved these changes Jun 30, 2026
Comment thread src/core/connection_pool.c Outdated
Comment thread src/core/connection_pool.c Outdated
@guhetier
guhetier enabled auto-merge (squash) June 30, 2026 20:29
@guhetier
guhetier merged commit 40f08c6 into main Jun 30, 2026
508 of 514 checks passed
@guhetier
guhetier deleted the fix-connection-pool-start-race branch June 30, 2026 21:16
@gaurav2699 gaurav2699 linked an issue Jul 2, 2026 that may be closed by this pull request
4 tasks
@gaurav2699 gaurav2699 linked an issue Jul 2, 2026 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment