Skip to content

Conversation

jthistle
Copy link
Contributor

@jthistle jthistle commented Sep 29, 2025

Heya, back again. Pretty sure this one is a bug. If the underlying call times out, it leads to an infinite loop since the TimeoutError is caught in the except SocketIOError block and ignored.

I would think that we want to reraise that TimeoutError. (And maybe all other SocketIOErrors too?)

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Sep 30, 2025

TimoutError is a subclass of SocketIOError.

Please for the future, if you are going to submit fixes to this project (or any project of mine, really), include unit tests that a) demonstrate that the bug occurs without the fix and b) pass when the fix is added.

@jthistle
Copy link
Contributor Author

Sure thing. I didn't see a CONTRIBUTING.md so I wasn't sure what guidelines you have for offering PRs, but happy to add tests for this.

As for the PR itself, perhaps I didn't make the issue clear. The docstring says that this function will raise TimeoutError if it times out. Current behaviour however is to catch TimeoutError and ignore it (because, as you say, it is a subclass of SocketIOError). However, if it were to behave as the docstring suggests it would reraise any TimeoutError.

Hope this makes some sense!

@miguelgrinberg
Copy link
Owner

Okay, sorry, I see the problem now. So what this PR is missing is a) similar fix on the async side, and b) unit tests. Is this something you can do?

@jthistle
Copy link
Contributor Author

Yes, I'd be happy to.

@jthistle jthistle force-pushed the simpleclient-call-no-timeout branch from 9effe4a to 354c2e2 Compare September 30, 2025 12:15
@jthistle
Copy link
Contributor Author

jthistle commented Sep 30, 2025

Ok, added tests which fail without the changes and pass with them.

e: will fix lint error when I get the chance in a couple hours. The other failure seems to be due to a cov rate limit.
ee: fixed!

@jthistle jthistle force-pushed the simpleclient-call-no-timeout branch from 354c2e2 to 353a8b9 Compare September 30, 2025 16:52
@miguelgrinberg miguelgrinberg merged commit a59c6f5 into miguelgrinberg:main Sep 30, 2025
@miguelgrinberg
Copy link
Owner

Thanks!

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