Don't set_result on completed futures. #2582
Conversation
jameshilliard
force-pushed
the
flush-recv-on-cancel
branch
from
2e23fb2 to
2ce8b21
Compare
pymodbus/transaction/transaction.py
Outdated
| @@ -153,6 +153,9 @@ async def execute(self, no_response_expected: bool, request: ModbusPDU) -> Modbu | |||
| return response | |||
| except asyncio.exceptions.TimeoutError: | |||
| count_retries += 1 | |||
| except asyncio.exceptions.CancelledError: | |||
| self.recv_buffer = b"" | |||
There was a problem hiding this comment.
Ah, yeah, I guess if we purge the buffer before each send we don't need to clean up on exceptions.
pymodbus/transaction/transaction.py
Outdated
| @@ -194,7 +197,8 @@ def callback_data(self, data: bytes, addr: tuple | None = None) -> int: | |||
| raise ModbusIOException( | |||
| f"ERROR: request ask for id={self.request_dev_id} but got id={pdu.dev_id}, CLOSING CONNECTION." | |||
| ) | |||
| self.response_future.set_result(self.last_pdu) | |||
| if not self.response_future.done(): | |||
There was a problem hiding this comment.
I think it would be more correct to notify the user by raising a ModbusIOException, because this is caused by a cancelation from outside pymodbus, and the app knows how to handle it correctly.
There was a problem hiding this comment.
notify the user by raising a ModbusIOException
If the future is already completed due to the future being cancelled I don't think we can set another exception like ModbusIOException on it. The execute function already raises a ModbusIOException after all retries are exhausted so I'm not sure we need to raise an exception from here.
this is caused by a cancelation from outside pymodbus
I'm not sure that's always the case, I think the pymodbus retry loop asyncio.wait_for also cancels the response_future when the timeout triggers.
If the app cancels the operation a ModbusIOException doesn't seem to make sense to me, on cancellation I think the normal CancelledError(which should AFAIU happen automatically from within the execute codepath waiting on the future even when cancelled externally by the app) is what is expected to be emitted since cancellation is not neccesarially indicative of any actual modbus error but may simply be the app abandoning the operation for whatever reason.
There was a problem hiding this comment.
I think we cannot just silently ignore a real error! if this happens it means that client handles 2 requests in the same call an that is. serious bug.
This is also correct if it happens for internal reasons, which I am pretty sure it does not.
There was a problem hiding this comment.
Maybe we should just log something? I mean if there isn't an active transaction I'm not sure what else we can do here as we don't have a future we can use to return an exception to right?
There was a problem hiding this comment.
This can also happen if a slave writes garbage on the line without a request right?
There was a problem hiding this comment.
The style in pymodbus is to raise a ModbuIOException, when something like this happens....typical users do not look in the log.
If a slave (server) writes data without being asked to, there are no request waiting, the loop is not active and data will be removed with next request.
jameshilliard
force-pushed
the
flush-recv-on-cancel
branch
from
2ce8b21 to
6c75239
Compare
jameshilliard
changed the title
jameshilliard
force-pushed
the
flush-recv-on-cancel
branch
from
6c75239 to
775a476
Compare
We need to guard self.response_future.set_result with a done() check to prevent asyncio.exceptions.InvalidStateError: invalid state errors.
jameshilliard
force-pushed
the
flush-recv-on-cancel
branch
from
775a476 to
3b3d0c7
Compare
We need to guard self.response_future.set_result with a
done()check to preventasyncio.exceptions.InvalidStateError: invalid stateerrors.Similar to #2581 I think we also need to flush the
recv_bufferif the transaction is cancelled by the caller(say by the caller executing a transaction inside of anotherasyncio.wait_forthat times out before theasyncio.wait_forinside of theexecutecall), in this case however we should re-raise the exception after the flush so that no retries are attempted.Since
self.response_futureis cancelled in this case we should also guard theself.response_future.set_resultcall.Should hopefully fix #2580 in combination with #2581.
fixes #2580