Bound the write in the synchronous serial client, so a request cannot hang forever - #3018
Open
tinegachris wants to merge 1 commit into
Open
Bound the write in the synchronous serial client, so a request cannot hang forever#3018tinegachris wants to merge 1 commit into
tinegachris wants to merge 1 commit into
Conversation
…orever connect() opened the port with a read timeout only, so pyserial applied its own default of write_timeout=None, meaning wait indefinitely. If the port could not accept the bytes, because the adapter had stopped draining its transmit buffer or hardware flow control was asserted, socket.write() never returned and never raised, and the calling thread was lost for the life of the process. SerialTransport already sets write_timeout on the same kind of port, so only the synchronous client was exposed. The read side was already bounded, which made this counter-intuitive: a device that answers nothing fails cleanly with a ModbusIOException, while a device that cannot accept bytes hangs forever, and timeout looks like it covers both. connect() now passes write_timeout alongside timeout. It is applied there rather than at construction because the client reopens the port itself when a request finds it closed, so a value set once would be lost on the first reconnection. write_timeout=0 would not do, since in pyserial that means non-blocking and send() would report a truncated frame as sent. send() converts the resulting SerialTimeoutException to ConnectionException without closing the port. A write timeout says nothing about the port being broken, and on RTU the line is shared, so dropping it would disturb slaves that are still healthy. This keeps the exception type pymodbus-dev#3008 established while respecting pymodbus-dev#3014, which stopped closing a serial port on no response.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ModbusSerialClient.connect()opens the port with a read timeout and nothing else, so pyserialapplies its own default of
write_timeout=None, which means wait indefinitely:send()then writes straight to that port. If the port cannot accept the bytes, because the adapterhas stopped draining its transmit buffer or hardware flow control is asserted,
self.socket.write()never returns and never raises, and the calling thread is lost for thelife of the process.
SerialTransportsets this on the same kind of port, apparently for the same reason:So the asynchronous path is protected and the synchronous one is not.
Two paths inside pyserial's posix
write()reach the same dead end, and neither returns nor raises.A partial write parks in
select.select(..., None)with no deadline. A write that cannot place asingle byte raises
EAGAIN, which the surroundingexcept OSErrorswallows before looping. Thesymptom is either a parked thread or one spinning on a core; from the caller's side both are
silence.
The read side is already correct:
timeoutreaches pyserial as the read timeout and_wait_for_data()is bounded by it, so a device that answers nothing fails cleanly with aModbusIOException. That makes the result counterintuitive. A silent device is handled properly,while a device that cannot accept bytes hangs forever, and
timeoutlooks like it should coverboth.
This is a different defect from #3008. That one is about an error the OS does report being
mishandled. Here the OS reports nothing at all, so there is no exception for that handler to see.
Reproduction
No hardware needed. A pty stands in for the adapter, nothing reads the far side, and its buffer is
filled, which is the state an adapter leaves behind when it stops draining.
Standalone script
On
dev(8aac385f), pyserial 3.5, Linux 6.18, three runs out of three:With this change, same script, three out of three:
The unpatched case is not slow, it is permanent. 20 s is only the patience of the harness.
One caveat if you run it yourself: the write only blocks if the buffer is still full at the instant
it is attempted. An occasional run slips an 8 byte frame through and returns promptly. Re-run it
rather than concluding the defect is absent.
Change
Two parts.
Bound the write.
connect()passeswrite_timeoutalongsidetimeout:self.socket = serial.serial_for_url( self.comm_params.host, timeout=self.comm_params.timeout_connect, + write_timeout=self.comm_params.timeout_connect, bytesize=self.comm_params.bytesize,Applied inside
connect()rather than at construction, because the client reopens the port itselfwhen a request finds it closed, so a value set once would be lost on the first reconnection.
write_timeout=0would not do here. In pyserial that means non-blocking, sowrite()returns ashort count,
send()reports the frame as sent, and a truncated request goes out. That is correctfor
SerialTransport, which buffers the remainder itself, but not for the synchronous client.Do not let the timeout drop the port.
except (BlockingIOError, InterruptedError): raise + except serial.SerialTimeoutException: + raise ConnectionException(str(self)) from None except OSError: self.close() raise ConnectionException(str(self)) from NoneThis second part is not cosmetic.
serial.SerialTimeoutExceptionsubclassesOSError, so withoutit the handler added in #3008 would catch a write timeout and close the port. That would
reintroduce serial port closing on a per-request timeout, which is what #3014 deliberately removed,
on the shared-bus reasoning argued in #2269. A write timeout says nothing about the port being
broken, and on RTU the line is shared, so dropping it would disturb slaves that are still healthy.
Keeping
ConnectionExceptionpreserves the exception type #3008 established.Only
send()needs it. pyserial'sread()returns short on a read timeout rather than raising, soSerialTimeoutExceptionnever arises on the receive path.Nothing changes for a healthy port, where the write completes long before the timeout.