Incorrect results when cancel_if_unresponded=true if a response is in the process of being parsed
- Dominant language
- C++
- Stars
- 282
- Forks
- 49
- PR merge metrics
- No merged PRs in 30d
Description
I've written the following (failing) test:
```cpp
void test_cancel_on_connection_lost_half_parsed_response()
{
// Setup
multiplexer mpx;
test_item item;
item.req.get_config().cancel_if_unresponded = false;
error_code ec;
// Add the request, write it and start parsing the response
mpx.add(item.elem_ptr);
BOOST_TEST_EQ(mpx.prepare_write(), 1u);
BOOST_TEST_EQ(mpx.commit_write(), 0u);
auto res = mpx.consume_next("*2\r\n+hello\r\n", ec);
BOOST_TEST_EQ(res.first, consume_result::needs_more);
BOOST_TEST_EQ(ec, error_code());
// Trigger a connection lost event
mpx.cancel_on_conn_lost();
BOOST_TEST(!item.done);
BOOST_TEST(item.elem_ptr->is_waiting());
// Simulate a reconnection
mpx.reset();
// Successful write, and this time the response is complete
BOOST_TEST_EQ(mpx.prepare_write(), 1u);
BOOST_TEST_EQ(mpx.commit_write(), 0u);
res = mpx.consume_next("*2\r\n+hello\r\n+world\r\n", ec);
BOOST_TEST_EQ(res.first, consume_result::got_response);
BOOST_TEST_EQ(ec, error_code());
// Check the response
BOOST_TEST(item.resp.has_value());
const node expected[] = {
{type::array, 2u, 0u, "" },
{type::simple_string, 1u, 1u, "hello"},
{type::simple_string, 1u, 1u, "world"},
};
BOOST_TEST_ALL_EQ(
item.resp->begin(),
item.resp->end(),
std::begin(expected),
std::end(expected));
}
```
In essence, add a request, write it successfully and start parsing its response. In the middle of this process, a connection loss happens. The adapter has already some data, which can't get discarded because adapters don't support it. Reconnection succeeds and the request is sent successfully and its response arrives. The final response contains the incomplete response received the first time, plus the complete one received the second time.
I don't think solving this is super important right now, but should be done at some point. We should probably disable unconditionally cancel the request (even if `cancel_if_unresponded=true`) if a response has received some data already. We could also think of something different, since `cancel_if_unresponded`, `cancel_if_not_connected` and `cancel_on_connection_lost` seem error-prone.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with test_cancel_on_connection_lost_half_parsed_response and trace the multiplexer paths through consume_next, cancel_on_conn_lost, and reset. Use the failing test to understand how partial response data survives reconnection; done means the final response contains only the complete response from the successful attempt and the regression test passes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, redis
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100