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

Fix asyncio.CancelledError on timeout #2503

Closed
wants to merge 2 commits into from
Closed

Conversation

philj56
Copy link
Contributor

@philj56 philj56 commented Dec 13, 2024

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?

@janiversen
Copy link
Collaborator

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.

@philj56
Copy link
Contributor Author

philj56 commented Dec 13, 2024

@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 TimeoutError. In the current code, if that happens and retries > 0, the future that just timed out will be waited for again and cancel immediately (if I understand this correctly).

@janiversen
Copy link
Collaborator

What is the other issue you refer to ? it's not one reported here.

@philj56
Copy link
Contributor Author

philj56 commented Dec 13, 2024

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 dev after #2501 got merged and immediately started seeing cancellation errors, so I assumed that was the cause.

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)

@janiversen
Copy link
Collaborator

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.

@janiversen
Copy link
Collaborator

#2500 is correct, because returning none is against the documentation (and philosophy of the API).

#2501 was made to cope with a reconnect problem, I have not seen side effects on this, but I might have overlooked some.

@philj56
Copy link
Contributor Author

philj56 commented Dec 13, 2024

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 dev again.

This PR is also to fix the reconnection not working - as far as I understand the code, the intention is to reset self.response_future to a fresh future any time await asyncio.wait_for(self.response_future) returns. In the current dev branch however, this won't happen if there's a TimeoutError here and a retry is attempted:

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 CancelledErrors with the async serial client when #2501 was merged, hence why I reverted it in this PR. I'm now not so sure that the revert is correct though; perhaps only the addition of the finally: block should be made, but I'd like to test again when I can.

@janiversen
Copy link
Collaborator

janiversen commented Dec 13, 2024

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.

@janiversen
Copy link
Collaborator

Closing this in favor of #2504.

Your PR´s are much valued, but this one was caused by a mistake by me, and I am solving this in #2504. I will ask for your test and opinion.

@janiversen janiversen closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants