Fixes for synchronized client and transaction handling #575
Conversation
|
I see that the diagnostic client in acdd9e9 caused those issues, that can easily be backed out. |
pymodbus/client/sync.py
Outdated
| self.close() | ||
| readsize = (f"read of {size} bytes" if size | ||
| else "unbounded read") | ||
| msg = (f'{self.__str__()}: Connection unexpectedly closed ' |
There was a problem hiding this comment.
fstrings are supported only in python3, this would essentially the PR valid for pymodbus 3.x. If you intend to support to python 2.x or want this to go in to pymodbus 2.5.0 please update accordingly. Otherwise please raise this against branch 3.0.0
There was a problem hiding this comment.
Thank you, good catch, this was a copy from the custom branch that Gapit uses, which is only 3.x. I don't have any thought of whether this should go into the 2.x or 3.x versions. Might as well update it to work for 2.x.
| # If size isn't specified read up to 4096 bytes at a time. | ||
| if size is None: | ||
| recv_size = 1 | ||
| recv_size = 4096 |
There was a problem hiding this comment.
This value is debatable. This would essentially block the recv till all 4096 bytes are received or timeout. Adding further to delay in reads.
There was a problem hiding this comment.
That's not how recv works. From the documentation:
$ pydoc socket.socket.recv | more
Help on method_descriptor in socket.socket:
socket.socket.recv = recv(...)
recv(buffersize[, flags]) -> data
Receive up to buffersize bytes from the socket. For the optional flags
argument, see the Unix manual. When no data is available, block until
at least one byte is available or until the remote end is closed. When
the remote end is closed and all data is read, return the empty string.
So it never blocks when a single byte is available, regardless of what buffersize is provided. I.e. it's a maximum limit, the minimum limit is always 1 regardless.
There was a problem hiding this comment.
@thormick That matches the underlying recv man page too:
https://man7.org/linux/man-pages/man2/recv.2.html
If no messages are available at the socket, the receive calls wait for a message to arrive, unless the socket is nonblocking (see fcntl(2)), in which case the value -1 is returned and the external variable errno is set to EAGAIN or EWOULDBLOCK. The receive calls normally return any data available, up to the requested amount, rather than waiting for receipt of the full amount requested.
There was a problem hiding this comment.
ah, I got this confused with Serial.Apologies for the mistake. Looks good.
There was a problem hiding this comment.
Actually, it's good that you picked on it, it made me gave it a quick look over and I realize now that ModbusTlsClient does the same, and now those two will be inconsistent. I don't have a setup ready for testing modbus TLS, so I'm not sure about touching that part of the code. Not necessarily a problem, though?
|
I've got the fstring thing sorted out, added and changed the unit tests a bit, and done a few other fixes too. But I'm not sure about intricacies regarding GitHub; is it correct to force push to the branch this PR is based off? Or should this PR be closed and another opened? The revised version resides in the branch at https://github.com/gapitio/pymodbus/tree/fix-sync-client-trans-2 for now. |
|
@thormick you can push the changes to the same PR, I will cherry-pick the changes to |
thormick
force-pushed
the
fix-sync-client-trans
branch
from
173a29a to
9462974
Compare
|
@dhoomakethu So done, I've rammed it in now. The major difference from the previous iteration is all the changes to the |
Performance: Patch out time.time() with itertools.count() in both test_client_sync and test_transaction; client.timeout now will be the number of reads being made + 1, this also speeds up execution of test suite (from 10.8s to 4.3s in local testing, with test.test_client_sync going from 2.1s to 0.05s). Coverage, pymodbus/client/sync.py: Add tests of idle_time(), debug_enabled(), trace(...), _dump(...) to testBaseModbusClient. Add test of _recv(None) to testTlsClientRecv. Invoke send/recv instead of _send/_recv, the former of which invoke the latter, increasing coverage. Expand test of serial client instantiation to ModbusSocketFramer; while this code does not make sense it does exist, and it should therefore be tested or removed. Test is_socket_open methods. Add test of connecting to an RTU serial client. Add test of serial client with client.timeout set to None. Fix name of method for testing tcp client repr to testTcpClientRpr. Coverage, pymodbus/client/sync.py: Add test of broadcast and handle_local_echo with wrong response. Missed statements go from 861 to 830.
Raise an error when the modbus unit unexpectedly closes the connection during a receive data operation without returning any data, and log a warning if it does return some but not all data before closing the connection. Detecting and handling this here ensures that ModbusTcpClient._recv doesn't needlessly loop until it times out after the stream is closed by the peer, and also makes it certain that a response of b'' from ModbusTcpClient._recv means there was a timeout and cannot mean that the peer closed the stream, as it could mean before. The previous behavior was to not identify the remote end closing the socket, triggering a timeout, in turn causing the transaction manager to treat it as the modbus unit returning a response with errors in it (raising InvalidMessageReceivedException). This will now raise a ConnectionException, which falls through to the client, bypassing the retry mechanisms. Note that https://docs.python.org/3/library/socket.html does not contain a full description on the socket.recv method; see also pydoc for socket.socket.recv.
Change the _sync method so that when no specific size is requested it fetches up to 4096 bytes instead of 1 byte at a time from the operating system. It's not clear what the reason was for setting it 1 byte. It was introduced in pymodbus commit: pymodbus-dev@9d2c864 Patch: pymodbus-dev@3b490fe But there's no rationale there for setting it to 1 byte specifically. While it may have been to handle some obscure operating system with a poor network stack implementation this should have been documented, as it is written it comes across as that the implementer didn't understand how the TCP/stream socket networking API works. Note that https://docs.python.org/3/library/socket.html does not contain a full description on the socket.recv method; see also pydoc for socket.socket.recv.
Synchronized TCP client that performs detail logging of network activity, for diagnosis of network related issues.
For TCP/IP when no data is received at all before a timeout the phrasing "incomplete message received" in error messages in ModbusTransactionManager._recv is misleading. This wording assertively states that a message has been received from the modbus unit, and when no actual data has received this would imply that a message with an empty payload has been received by the application, which is not possible with TCP/IP. More likely it's just a timeout, without any data at all having been received from the modbus unit.
It's not just not receiving the right number of bytes when there is a known number of bytes to read that is an issue, but also when there is no response at all to a read of None bytes (_recv isn't invoked unless one expects a response).
Always close connection on no response/error response from modbus unit. This attempts to resolve issue pymodbus-dev#538, where the response of a previously timed out request over synchronized TCP is being returned as the response to the current request, by leaving the connection open only if there is no unanswered response. This commit only touches the code for synchronized clients, and only seeks to resolve it for TCP based clients, though the situation for UDP clients should improve as well.
thormick
force-pushed
the
fix-sync-client-trans
branch
from
9462974 to
24c0ffc
Compare
|
Made some small additions to the unit tests. Commit-to-commit diff relative to my Jan 8 push here. |
|
SonarCloud Quality Gate failed.
|
|
@dhoomakethu Thank you very much, glad you could make use of it all. |
3 participants
Fixes in the synchronized client and the parts of the transaction manager used by the synchronized client.
This has all the work I did in the sync client for gapit.io. Let me know if this should be split up, for instance with just the #538 fix. The other commits are for issues I discovered while digging to find and fix that issue.
Sorry for not getting around to this before, things have popped up so that even if this sat almost ready and just needed a once or twice over it still got punted a month, but at least now it's presentable enough for a PR.
No sweat about merging all of this, gapit.io is using a fork for pymodbus, has been doing so for a long time and can do so for a while longer.