-
Notifications
You must be signed in to change notification settings - Fork 951
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
Fix asyncio.CancelledError on timeout #2503
Conversation
Please add a debug log, so we can see what happens. Your fix is not correct, it basically removes the renewal of the future, which is needed. |
@janiversen I can on Monday and I'm happy to do so, but I'll be busy until then. I don't think this does remove the renewal of the future though, it adds it in the case that there's a |
What is the other issue you refer to ? it's not one reported here. |
It's the PR that just got merged to fix the issue in the discussion at #2499 . Having looked at that again though, I'm not sure that these two things are 100% related - I just happened to pull I can investigate further next week if you're happy to just let this PR sit until then (it'd be Tuesday actually, rather than Monday) |
Are you talking about log entries or result.iserror() ? The fix will create more log entries due to the automatic reconnect, which was not working correctly before. |
Sorry for the confusion - Yeah, this PR is completely unrelated to #2500, it's about #2501. #2501 just happened to be merged at the same time, so I saw it when I tried This PR is also to fix the reconnection not working - as far as I understand the code, the intention is to reset while count_retries <= self.retries:
self.pdu_send(request)
if no_response_expected:
return None
try:
response = await asyncio.wait_for(
self.response_future, timeout=self.comm_params.timeout_connect
)
self.count_no_responses = 0
self.response_future = asyncio.Future()
return response
except asyncio.exceptions.TimeoutError:
count_retries += 1
# No reset of self.response_future, so next time round the loop the wait_for will fail with CancelledError I think this fixes the same issue that #2501 did, but I got lots of |
You have a conflict with dev, however I do not think it is something serious. #2501 is simply WRONG ! it was made a late night to get the server working. I am working on a PR, that will solve the problem and make a more correct implementation. Sorry for the noise. Your fix is not bad, but also not complete, since the future is used for both client and server. |
The change in #2501 causes constant cancelled errors for me, using the async serial client. I think this is the proper fix, but it should be tested before merging.
@alexis-care Would you be able to try this branch and see if it also fixes your issue?