Test: Make the connection-reuse tests exercise reuse again - #645
Conversation
The destructor closed the listener and the connection socket on the test thread while the worker could be closing the same listener at the end of serve(). Asio's close is not thread-safe. The worker can read the listener's descriptor just before the destructor's close clears the reactor data, then dereference that null data in epoll_reactor::deregister_descriptor. Under parallel load, unbounded_interim_responses_are_rejected crashes this way about once in a hundred runs. Take one mutex around every close and reassignment of the listener and socket that both threads reach. The destructor also shuts the socket down before closing it. On Linux a close from another thread does not wake a handler blocked reading the socket, so without the shutdown, teardown would wait for the client to close its end.
…tion Both stale-connection download tests shut the server side down right after the keep-alive metadata response. Since the transport started checking an idle connection before reusing it, a connection whose peer has closed is discarded rather than reused. So neither test reached the reused-connection retry that retry_failed_reused_connection enables: the download went straight to a fresh connection. Hold the connection open after the metadata response, take the download request on it, then drop it unanswered. The reuse check passes, the reused request fails, and the transport retries on a fresh connection. Both tests assert that the download request arrived on the reused connection. The reconnect-failure test's only connect used to come within a millisecond of the server closing its listener. On hosts whose listener teardown is asynchronous, WSL2 mirrored networking among them, that connect was accepted and then reset, which surfaced an io failure instead of the expected connect failure. The reconnect now follows the retry backoff, and the test holds it at its connecting phase until the port refuses, so it fails at connect on every host. The fixture's finished() accessor lost its only caller and is removed.
…ests idle_connection_pool_cap_can_disable_reuse, expired_idle_connection_is_not_reused and idempotent_call_recovers_from_a_stale_cached_connection each had the server close the first connection right after answering it. The transport's closed-peer check then discards that connection before reuse. So the second request always went out on a fresh connection, whether or not the idle-pool cap, the idle-age expiry or the JSON-RPC stale-connection replay worked. The pool tests' server now keeps answering on the first connection until the client closes it, so the pool alone decides whether the connection is reused. The JSON-RPC fixture holds the first connection until the second call arrives on it, then drops that call unanswered, so the client has to replay it on a fresh connection. The server therefore receives three requests instead of two.
| return; | ||
| request_arrived = !read_request_header(socket).empty(); | ||
| boost::system::error_code ec; | ||
| socket.close(ec); |
There was a problem hiding this comment.
[P2] Serialize this socket close with fixture teardown
This helper calls socket.close() without the fixture’s _close_mutex. If metadata fails or times out, client destruction or the server destructor’s new shutdown() can wake this blocked read while teardown also closes the same socket. The worker therefore still permits concurrent Asio closes—the crash class this PR addresses.
Please route this close through the fixture’s synchronized close operation, or leave closing to the worker under that mutex.
There was a problem hiding this comment.
Good catch. The close only overlaps teardown when the test is already failing, but then it's the double close the first commit removes, and it would turn an assertion failure into a crash of the whole test_fc run.
Fixed in 4e5912f: the helper no longer closes. Once the reused request arrives it only shuts the connection down, which the client still sees as the stale-connection EOF. If the read fails, it returns without touching the socket. Every close now happens in the fixture under _close_mutex. The same unlocked close is also gone from truncated_fixed_length_response_is_rejected_and_removed, whose client can see the FIN from its shutdown before that close runs.
serve_metadata_then_drop_next_request closed the fixture's socket without its close mutex. In a test that is already failing, the client may never reuse the pooled connection. The helper then stays blocked reading until teardown, where client destruction or the destructor's shutdown wakes it. Its close then races the destructor's close, and the resulting crash takes down the whole test_fc run. The helper now only shuts the connection down, and only once the reused request has arrived. The client still sees the EOF of a stale connection. If the read fails, the helper returns without touching the socket. truncated_fixed_length_response_is_rejected_and_removed closed the socket right after shutting down its send side, and its client can see that FIN and reach teardown first. It now leaves the close to the fixture as well.
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed the full diff at 4e5912f. The earlier double-close finding is addressed, and I found no new actionable issues. Validation was static; PR-head tests were not run locally.
2 participants
Five connection-reuse tests stopped exercising reuse when the transport rewrite added a check before reuse:
healthy_for_reusepeeks the idle socket and discards a connection whose peer has already closed it. Each test had its server close the first connection right after answering it. So the second request always went out on a fresh connection, whatever the feature under test did.stale_metadata_connection_retries_download_on_fresh_connectionretry_failed_reused_connectionenablesstale_metadata_reconnect_failure_cleans_up_safelyidle_connection_pool_cap_can_disable_reuseexpired_idle_connection_is_not_reusedidempotent_call_recovers_from_a_stale_cached_connectionWith all four features disabled, the versions of these tests on master still pass.
The same gap is why
stale_metadata_reconnect_failure_cleans_up_safelywas the onetest_fccase still failing on WSL2 after #633. Its only connection attempt was a fresh connect within a millisecond of the server closing its listener. On WSL2 mirrored networking a closed listener keeps accepting for several milliseconds and then resets. So the transport reportedresponse header read failed: Connection reset by peer(an io failure) rather thanFailed to connect.The fix
Each server now keeps the first connection open until the client acts on it, so the feature under test is the only thing that decides whether the connection is reused.
retry_exhaustedwrapper, asfresh_download_connection_failure_is_not_retrieddoes.In the failure test, the reconnect now follows the 100 ms retry backoff, so it is already well past the WSL2 window. To make it deterministic regardless of scheduling, the test's status callback holds the retry at its
connectingphase until the port refuses a probe connection. On Linux and macOS the first probe is refused, so nothing waits.With the same four features disabled, all five tests now fail on every host.
Fixture teardown race
scripted_http_server's destructor and its worker could both close the listener at once, and asio's close is not thread-safe: the worker dereferenced reactor data the destructor had just cleared. Under 16-way parallel load, the unchangedunbounded_interim_responses_are_rejectedcrashed this way 18 times in 2000 runs. The reworked pool-cap test crashed 6 times, because its worker now closes the listener just as the test ends. The first commit serializes those closes under a mutex. It also has the destructor shut the socket down, so a handler blocked reading it wakes instead of waiting for the client to close its end.scripted_http_server::finished()lost its only caller and is removed.