Closed mrctrifork closed 11 months ago
Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.
Thanks for integrating Codecov - We've got you covered :open_umbrella:
Hi, thanks a lot for your feedback.
I am newish writing python. My background is more TypeScript / Kotlin during these last years. I lack knowledge on how async/await actually works in python so I just attempted to make it work.
In regards this:
After writing the comments, I'm now wondering whether we need this timeout at all: Shouldn't the TCP connection be closed, when your server reboots, such that we get an asyncio.IncompleteReadError and the run() coroutine raises an exception? If that doesn't work, maybe we should try to configure the TCP keepalive on the network socket?
Well – Your judgement will be better than mine. My programming background is mostly in other languages so I just don't know the python internals about async/await. I just figured that the "program got stuck because the queue was not closed yet the connection got dropped".
I'll go over your comments when I get the chance and ping you once the changes have been submitted.
Again; thank you for your time and feedback and I'll take this as far as I can :)
We'll have the official test some time from now :), once that happens I'll consider the new approval to be working.
And you're right. CI/CD is failing now so I'll take a look at that tomorrow
I can confirm it did the trick!
I can confirm it did the trick!
Then, we can remove the timeout parameter, can't we?
We can also keep it, but than we should document in the docstring of the initmethod (via an @param timeout ...
line) that it should not actually be required and if it is set, it will require regular KNX bus activity.
I took the liberty to do some small improvements to the code style and add a test for the new behaviour. Please take a look, if this still is in your sense; then I will merge the PR.
Good afternoon, I've been away from the computer for a bit. Thank you for your time to make these improvements and looking at the changes.
The other day I was answering your comment but I did not hit send in the end; shut down the machine and forget about everything.
Then, we can remove the timeout parameter, can't we? We can also …
I meant that I just tried with my changes – didn't try with the TCP keepalive options
First of all, thank you for creating this library. It's nice and easy to use.
Issue
I have a server running
knxd
that reboots once a day and I've noticed that if one usesiterate_group_telegrams
andknxd
reboots it leaves the coroutine hanging; awaiting for a message that will never come since the connection was dropped. This is caused because there were no timeouts in place.Suggested fix
I've added an optional parameter to the class
KNXDConnection
that can be used as a second argument toasyncio.wait_for
and it is used in places where we are reading fromknxd
. The timeout errors will allow us to break out of thewhile True
polling statements.If the
timeout
isNone
old behavior is used instead.Thank you for taking a look at this.
Miquel