[modbus] Eventually close borrowed connections also when config changes #5088
Conversation
9fcd0a5 to
4ef8d43
Compare
9037568
left a comment
There was a problem hiding this comment.
LGTM.
Should we wait for feedback from the original poster of the defect before merging?
|
Let's wait. Looking at the test with fresh eyes I also noticed something peculiar -- I need to check it out further |
|
OK while looking at the "expected results" I found out additional bug with transaction IDs. Race conditions were possible which lead to too eager discard of the messages. See the commit messages for more information. I had to modify the expected results and I now think those make more sense. I added comments to the test to make the reasoning more transparent. Do you want me to create separate bug for the transaction ID issue? Best, (old download link for reference ) |
|
(comment removed, new version below) |
|
Fixed some logging related stuff in 823e664, 48a3ee3 and 175604c. Testing with the community on this thread. User sipvoip has several "transaction id errors", not sure what causes them but good to test if the fixes in this branch help. Latest version attached |
|
Forgot to update community user sipvoip case here: we managed to have it working successfully in the end. We seemed to fix the issues with his modbus slave by re-using the same connection and not closing after every request. The issues were apparently how the slave handles connection disconnect sequence (SYN/FIN etc.). Maintainers, please let me know if I can help reviewing this PR in any way. Also, please see comment above regarding the transaction ID issue I found out (and fixed in this PR as well). Do you want me to separate that to its own PR and issue? @ManniAT, have you had to test this one out? Best |
|
I don't see a need for an additional PR for the transaction ID issue. Let's just include it here. |
9037568
left a comment
There was a problem hiding this comment.
This just needs an Eclipse format and it'll be good to go.
Let me know when you're ready to have it merged...
Also regression test introduced. Fixes openhab#5078 Signed-off-by: Sami Salonen <ssalonen@gmail.com>
No clashes were possible even before due to the limit of single connection / endpoint; and the fact that transaction is not modified before connection is acquired. However, this now makes this code more robust (independent) against any changes in pooling settings.
Now when requests are retried, the transaction id is incremented every time. I think it’s always safe to increase the id, but leaving it as is might be risky (some slaves might not like it, for example). Increasing the ID is more like “fire and forget” kind of behaviour, if the original request fails, we make completely new one instead of repeating the old one.
Fixed test case: The expected results were counter-intuitive due to the transaction ID bugs Race condition with transaction IDs: Previously the code checked response transaction id against static transaction id (that is, ever increasing global counter). When multiple requests are processed at the same time (testConfigUpdatedWhilePolling), the logic fails. Instead, we can just query the transaction id from the actual request that wast sent.
Now the transaction is synchronized also during the "transaction id mismatch" logging that includes object id of the request as well.
6fb75ee to
ca19abe
Compare
|
Rebased the code and run the Eclipse format. |
|
@9037568 Everything good to merge? |
|
Yep, good to go if you're ok with it. |
|
I'm good. Seems like we are not getting response from original reporter. |
|
Thanks! |
Also regression test introduced.
Fixes #5078
Signed-off-by: Sami Salonen ssalonen@gmail.com
Pre-compiled version for testing available below in the new comment
(old download link for reference
)