pymodbus-dev / pymodbus

A full modbus protocol written in python
Other
2.16k stars 891 forks source link

Remove pointless try/except in polling_task #2091

Closed alexrudd2 closed 3 months ago

alexrudd2 commented 3 months ago

See https://github.com/pymodbus-dev/pymodbus/pull/2090. This PR goes a step further.

This code is a stack of clever hacks. https://github.com/pymodbus-dev/pymodbus/blob/d6704a2ff9abddd0de29d6d50a1650a9d71c084c/pymodbus/transport/serialtransport.py#L45-L48

The purpose of this code is to cancel self.poll_task() and then set it to None afterwards.

Because the code is sync, await cannot be used to ensure the cancellation.
Hack #1 - use ensure_future

Then ruff complains with RUF006. This could have been safely ignored - nobody cares if the GC cleans up, since it's immediately set to None. Instead... Hack #2 - use self.future to hold a reference.

Next mypy complains that self.future is untyped. Hack #3 - add an annotation to the constructor.

Looking at polling_task, I see no purpose to catching asyncio.CancelledTask. All it does is call self.close().... which is the very code that cancelled it! I think the whole try/Except and stack of hacks can be removed. :)

alexrudd2 commented 3 months ago

If the try/except is still wanted, the solution is to just ignore ruff and delete self.future

asyncio.ensure_future(self.poll_task)  # noqa: RUF006
janiversen commented 3 months ago

If we can avoid the try/cancel I am all in favour, I just cannot see how the pyserial object is being closed if the task is cancelled.

janiversen commented 3 months ago

hmmm the failing test in 3.10 is a tcp test, I am confused

alexrudd2 commented 3 months ago

Is there any way that poll_task gets cancelled except explicitly in self.close()?

janiversen commented 3 months ago

I am not sure if asyncio.protocol cancels running tasks in some cases, apart from that I think you are correct that it is only self.close() that does it.

alexrudd2 commented 3 months ago

hmmm the failing test in 3.10 is a tcp test, I am confused

I've noticed we have a rare flaky test again, but have not investigated further.

janiversen commented 3 months ago

I will keep an eye open, I have had seen it lately.