Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(plc4go/modbus): Delete elements in the loop, and the index is dec… #1028

Merged
merged 2 commits into from
Jul 12, 2023

Conversation

hongjinlin
Copy link
Contributor

In some batch read/write cases, because the element is deleted in the loop, but the index is not decremented by 1, the program freezes, and even the timeout does not work.

@hongjinlin hongjinlin requested a review from sruehl July 11, 2023 10:01
Copy link
Contributor

@sruehl sruehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test for this change. It is such a central change which needs a test to proof the failure before the change and that it is working right after the change. The test then I can try out on the current develop to understand what exactly is happening

@sruehl
Copy link
Contributor

sruehl commented Jul 12, 2023

Small note: you can add a testcase in DefaultCodec_test.go before line 686

@sruehl
Copy link
Contributor

sruehl commented Jul 12, 2023

And looking at it: And easy fix would be to break the loop after we found someone who accepted the message. There should be only one at a time accepting the message I think

Copy link
Contributor

@sruehl sruehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could replicate the problem and verify that your fix fixes it. Thanks for the PR 🎉

@sruehl sruehl merged commit a359a2f into develop Jul 12, 2023
6 checks passed
@sruehl sruehl deleted the fix/PLC4GO-BATCH-WRITE-ERROR branch July 12, 2023 08:59
@hongjinlin
Copy link
Contributor Author

I could replicate the problem and verify that your fix fixes it. Thanks for the PR 🎉

Thanks for reviewing and adding testcase 😎 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants