Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove pointless try/except in polling_task #2091

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

alexrudd2
Copy link
Collaborator

See #2090. This PR goes a step further.

This code is a stack of clever hacks.

if self.poll_task:
self.poll_task.cancel()
self.future = asyncio.ensure_future(self.poll_task)
self.poll_task = None

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
Copy link
Collaborator Author

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

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your idea is not bad, but it seems you broke something so that the reconnect does not run.

The cancelled most likely comes from the same code, as you write, but it might also come from transport (I think),

@janiversen
Copy link
Collaborator

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
Copy link
Collaborator

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

@janiversen janiversen merged commit bb888c6 into dev Mar 7, 2024
1 check passed
@janiversen janiversen deleted the CancelledError branch March 7, 2024 17:45
@alexrudd2
Copy link
Collaborator Author

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

@janiversen
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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

janiversen pushed a commit to dnssoftware/pymodbus that referenced this pull request Mar 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants