EventQueue: Old pointers of sibling were not cleared #9144
Conversation
When adding sibling at the head of linked list, the head if pointing
to something in linked list was not updated, hence a loop was formed
in linked list
Element0 - First addition to linked list
Element1 - Has higher delay hence added to back
0 ->(next) 1
Element2 - Delay is same as Element0, hence should be sibling of 0
Shall be added at head
Expected:
2 ------------->(next) 1
|(sibling)
0
Bug: (Resolved with this)
2 ------------->(next) 1
|(sibling)
0 ------------->(next) 1
If we add more elements and next pointer of sibling is updated, old
references will cause issues
Element3 added
Expected:
2 ------------->(next) 3 ------------->(next) 1
|(sibling)
0
Bug: (Resolved with this)
2 ------------->(next) 3 ------------->(next) 1
|(sibling)
0 ------------->(next) 1
***Both siblings here point to different next***
|
@deepikabhavnani Would it be possible to extend the event unit tests to make sure the problem is solved ? |
Yes. Wanted to confirm first if next should be |
|
Additional commit to correct traverse in destructor |
In `equeue_destroy` the external loop was for main events linked list and internal loop for siblings. Siblings start was not initialized correctly for each main link
deepikabhavnani
force-pushed
the
equeue_cancel_issue
branch
from
2f3df8a to
01b2530
Compare
deepikabhavnani
force-pushed
the
equeue_cancel_issue
branch
from
a8947d5 to
12623ac
Compare
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
cmonr
added
ready for merge
release-version: 5.11.1
needs: review
and removed
needs: CI
ready for merge
labels
|
Waiting on a final review by @geky, but otherwise should be good to go. |
|
Sorry for such a late review, this looks like a perfect fix 👍 Looking into it, the equeue relies on sibling's next pointers being NULL here: Unfortunately it was a hard catch because it is only an issue when canceling siblings which also have a sibling. In this case the equeue tries to update the next event's reference when sibling is not NULL, which is wrong and corrupts the data structure. The is then made worse because events references can end up unused in a lot of use cases, making the corruption go unnoticed.
Thanks for the fix! After looking at the code again I'm embarrassed the equeue has been used for this long without some sort of design doc, sorry you guys had to reverse engineer all of that. |
|
My mistake. Starting CI has become a reflex now. Updated status accordingly. |
Description
When adding sibling at the head of linked list, the head if pointing to something in linked list was not updated, hence a loop was formed in linked list
Element0 - First addition to linked list
Element1 - Has higher delay hence added to back
0 ->(next) 1
Element2 - Delay is same as Element0, hence should be sibling of 0, shall be added at head
Expected:
2 ------------->(next) 1
|(sibling)
0
Bug: (Resolved with this)
2 ------------->(next) 1
|(sibling)
0 ------------->(next) 1
If we add more elements and next pointer of sibling is updated, old references will cause issues
Element3 added
Expected:
2 ------------->(next) 3 ------------->(next) 1
|(sibling)
0
Bug: (Resolved with this)
2 ------------->(next) 3 ------------->(next) 1
|(sibling)
0 ------------->(next) 1
Both siblings here point to different next
Resolves: #8664
Pull request type