Skip to content

Conversation

@ofekby
Copy link

@ofekby ofekby commented Jul 6, 2025

This pull request makes a small but important change to the retry logic in the __retry method of the src/aioice/stun.py file. The change ensures that the TransactionTimeout exception is not redundantly set if the future is already completed.

  • src/aioice/stun.py: Modified the condition in the __retry method to check if the future (self.__future) is not already done before setting the TransactionTimeout exception. This prevents unnecessary operations and potential errors.

We experience this case in production here is the stacktrace:

Traceback (most recent call last):
  File "uvloop/cbhandles.pyx", line 253, in uvloop.loop.TimerHandle._run
  File "[masked]/aioice/stun.py", line 330, in __retry
    self.__future.set_exception(TransactionTimeout())
asyncio.exceptions.InvalidStateError: invalid state

@jlaine
Copy link
Collaborator

jlaine commented Nov 28, 2025

This looks like a good catch, could you please prepare a unit test which exercises this scenario?

@jlaine jlaine added the changes requested Some changes are required before the PR can be merged. label Nov 28, 2025
@ofekby
Copy link
Author

ofekby commented Dec 1, 2025

This looks like a good catch, could you please prepare a unit test which exercises this scenario?

Thanks! @jlaine
Added a test to check the race condition where the message is resolved right before the retry.
Fortunately, the event loop behaves deterministically here - __retry runs before the cancellation, so the bug is reliably reproduced.

The test fails consistently when the future-guard fix is commented out, confirming that the test reliably captures the issue.

To detect the raised exception (InvalidStateError, previously unhandled), I added CollectExceptionsHandler, new_collect_exceptions_handler, and detect_exceptions_in_loop to keep the test clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Some changes are required before the PR can be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants