Fix duplicate stream detection in mpas_stream_list_insert #1353
Conversation
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
from
5e91e71 to
9b7263a
Compare
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
3 times, most recently
from
5ebe42b to
838fa77
Compare
There was a problem hiding this comment.
Aside from a couple of comments that I've left, it might be worth adding at least one test to verify that we can successfully remove items from the middle or the end of a list. Currently, it seems that we only test:
- Removing the only item (head) of a list
- Attempting to remove an item from an empty list (which should yield an error)
- Attempting to remove a non-existent item from a list (which should yield an error)
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
from
9b63241 to
7c580b8
Compare
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
2 times, most recently
from
63be121 to
dec7767
Compare
|
@amstokely I've left just one more comment regarding changes to the
|
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
from
72d6036 to
7a87f04
Compare
Unit tests were added for the mpas_stream_list module, covering stream list creation, insertion, querying, and removal. The tests fail when inserting duplicate streams adjacent to each other, either as the first or last stream in the list. The bug is in the MPAS_stream_list_insert subroutine in the mpas_stream_list module, which does not correctly handle duplicates in these cases.
The original code allowed adjacent duplicate streams to be inserted into the list, which caused incorrect behavior when adding a duplicate stream next to an existing one. The bug was fixed by updating the insertion logic to properly reject adjacent duplicate streams. The new code checks for duplicates during insertion and prevents adding the stream if it is already in the list, even if adjacent. The mpas_test_insert_duplicate_at_begin and mpas_test_insert_duplicate_at_end tests in the mpas_stream_list test suite confirm that these changes fix the bug.
There was a problem hiding this comment.
Can you try again to rework the commit history? Currently, 1b24f2f not only fixes the duplicate insertion bug, but it also makes many changes to the unit tests introduced in f8eb09c. Also, can you add some details to the commit message that is currently in 7a87f04 and ensure that any changes to the unit test module are moved into the first commit?
I'm working on it right now! |
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
from
7a87f04 to
214a6e7
Compare
Fix bug in mpas_stream_list_insert that could unlink the head node when a duplicate stream was inserted. Moved nullify(stream % next) calls into the relevant conditional blocks to ensure new streams are only linked after passing duplicate checks. Prevents inadvertent modification of the list when duplicate insertions occur.
amstokely
force-pushed
the
framework/duplicate_stream_fix
branch
from
214a6e7 to
3f9ead8
Compare
This PR fixes a bug in the
mpas_stream_listmodule where adjacent duplicate streams were allowed to be inserted into the list. The issue was in theMPAS_stream_list_insertlogic, which has been updated to properly reject adjacent duplicates. The fix has been confirmed by thempas_test_insert_duplicate_at_beginandmpas_test_insert_duplicate_at_endtests. Additionally, the unit test suite covers all core subroutines in thempas_stream_listmodule, ensuring there are no other similar bugs.