boostorg / boostorg/redis

Incorrect results when cancel_if_unresponded=true if a response is in the process of being parsed

Open
#307 4 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.