fix: take indices to messages as a hint #5683
Conversation
By this, do you mean the fact that the circular buffer could have shifted underneath? That could have also caused this, that was what I had in mind looking at this, and thought of a similar solution to yours (i.e. replace index with the message pointer and check based on that). Other than that I didnt quite think up any other possible causes. Of course, supposing this was the problem, I tried forcing replication and still didn't achieve much, though maybe a hardcoded test with this could replicate it. I think your tests do not test when the buffer is full - it might be something to look at. |
|
(a side note) 6430682 ULL suffix should be in c++11, i did not see you use the c++23 z suffix. (If needed there's also the possibility of using custom suffixes - user literals) (Maybe im just seeing sth wrong in github) |
Yes. Probably a bit more general than reentrancy, but related in that assumptions about indices are invalidated.
I could test
Sure.
|
Didn't know that there were some things like that with regards to size_t type, granted, unless it is larger than unsigned long long, which the verbiage in standard seems to allow, using ull should be fine to cover possible issues with regards integer conversion and comparison problems. I didn't know the underlying issue and just saw the commit name and the code, so thought something didnt add up, thanks for the explanation. But yeah, this isnt really much needed in the tests 👍 |
If you only have an argument of |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return i == 0; | ||
| }) | ||
| .has_value()); | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| }) | ||
| .value(), | ||
| 1); | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| }) | ||
| .value(), | ||
| 2); | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| }) | ||
| .has_value()); | ||
| // correct hint | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| }) | ||
| .value(), | ||
| (Pair{0, 1})); | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| .value(), | ||
| (Pair{1, 2})); | ||
| // incorrect hint | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| }) | ||
| .value(), | ||
| (Pair{0, 1})); | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^| .value(), | ||
| (Pair{5, 6})); | ||
| // oob hint | ||
| EXPECT_EQ(queue |
There was a problem hiding this comment.
warning: unchecked access to optional value [bugprone-unchecked-optional-access]
EXPECT_EQ(queue
^
#2804 shows that using indices across different channels can be buggy. In
ChannelViewwe maintain two buffers - the buffer for the virtual channel and the buffer for the messages. But more importantly, we blindly re-use indices frommessageReplacedevents from the underlying channel to index into the virtual channel. Both channels don't necessarily have the same messages (e.g. when using filters). Thus, we can't re-use the indices. However, as time shows, in most cases, the index we get will be correct, as most of the time, that index is zero (i.e. no messages have been added since).This PR adds a "middle-way" to solve this.
LimitedQueuecan now take ahintto find and replace items. If that hint points to the expected item, no additional work is required. However, if that hint doesn't match, the whole buffer is searched for the desired item.Fixes #2804 (hopefully, I couldn't replicate it - if this doesn't solve it, we have reentrancy issues when replacing messages)