fix: record match results synchronously to eliminate race condition (#13)#14
fix: record match results synchronously to eliminate race condition (#13)#14rholshausen wants to merge 1 commit into
Conversation
) Previously, match results were sent via an async MPSC channel and processed by a separate event loop task. Under CPU pressure, the Tokio scheduler could delay the event loop long enough that a caller invoking pactffi_mock_server_matched() / pactffi_mock_server_mismatches() immediately after receiving the HTTP response would see an empty matches list and incorrectly report the request was never received. Fix by passing Arc<Mutex<Vec<MatchResult>>> directly into handle_request() and updating matches inline before returning the HTTP response. The match is now committed to the mutex before a single byte of the response reaches the client, so there is no window for a race regardless of scheduling. The RequestMatch event variant is removed from MockServerEvent as it is no longer needed. The event channel is now used only for RequestReceived (metrics) and ServerShutdown. Fixes: #13 Related: pact-foundation/pact-reference#535 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition where match results could be observed as empty immediately after an HTTP response under scheduler/CPU pressure, by recording match results synchronously in the request handler instead of via an async event loop.
Changes:
- Pass
Arc<Mutex<Vec<MatchResult>>>intohandle_request()and push match results inline before returning the HTTP response. - Remove
RequestMatchfromMockServerEventand simplify the event loop to only handle metrics/shutdown events. - Update Hyper server tests to validate synchronous match recording and adjust event expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pact_mock_server/src/mock_server.rs | Removes RequestMatch event handling and wires shared match storage into server creation. |
| pact_mock_server/src/hyper_server.rs | Records match results synchronously in handle_request(); updates bind functions and tests accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ConnectionFailed(String), | ||
| /// Request received with path | ||
| RequestReceived(String), | ||
| /// Result of matching a request | ||
| RequestMatch(MatchResult), | ||
| /// Server is shutting down | ||
| ServerShutdown |
|
It looks OK to me, however it breaks the public API so will need a version bump I guess, and then an update in the core that pulls it in? |
|
Created a repro for the original issue in a branch on a pact-js fork pact-foundation/pact-js@master...YOU54F:pact-js:refs/heads/repro/issue-1713 managed to reproduce in CI, and locally with a resource constrained runner (vm with 2 cpu's) I have created 2 shared libs both for linux (aarch64 musl and x86_64 glibc) for testing which pull in this fix into the pact_ffi and builds it https://github.com/YOU54F/test/releases/download/0.0.0/libpact_ffi.so Now unable to reproduce the error locally or in CI, so looks good from my perspective. |
JP-Ellis
left a comment
JP-Ellis
left a comment
There was a problem hiding this comment.
Looks fine to me, with just one nit: I really don't like unwrap 😆 Even if you want to keep it, can you use expect with an appropriate message?
| // state immediately after the client receives the HTTP response, with no race against the | ||
| // async event loop. | ||
| { | ||
| let mut guard = matches.lock().unwrap(); |
There was a problem hiding this comment.
The unwrap makes me uncomfortable... While it is unlikely that the mutex gets poisoned, it would probably still be nicer to return an error if it fails.
Previously, match results were sent via an async MPSC channel and processed by a separate event loop task. Under CPU pressure, the Tokio scheduler could delay the event loop long enough that a caller invoking pactffi_mock_server_matched() / pactffi_mock_server_mismatches() immediately after receiving the HTTP response would see an empty matches list and incorrectly report the request was never received.
Fix by passing Arc<Mutex<Vec>> directly into handle_request() and updating matches inline before returning the HTTP response. The match is now committed to the mutex before a single byte of the response reaches the client, so there is no window for a race regardless of scheduling.
The RequestMatch event variant is removed from MockServerEvent as it is no longer needed. The event channel is now used only for RequestReceived (metrics) and ServerShutdown.
Fixes: #13
Related: pact-foundation/pact-reference#535