Fix #3588 #3598
Fix #3588 #3598
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3598 +/- ##
=========================================
Coverage ? 77.25%
=========================================
Files ? 103
Lines ? 13723
Branches ? 1931
=========================================
Hits ? 10602
Misses ? 2461
Partials ? 660
Continue to review full report at Codecov.
|
| room_is_empty = not bool(peer_ids & member_ids) | ||
| if room_is_empty or last_ex: | ||
| if last_ex: | ||
| raise last_ex |
There was a problem hiding this comment.
What's the point of raising last_ex inside for retry loop? It's the same as not handling the exception at all. The raise last_ex inside else of for loop (or in a if last_ex later) is a pattern to allow swallowing exceptions a given number of times, and re-raising them iff for couldn't succeed (break) on the retries. Re-raising inside the loop is the same as not catching it
There was a problem hiding this comment.
The idea is to only raise the exception once the retries have been exhausted, as the server response might change in the meantime.
There was a problem hiding this comment.
I know. That's the pattern, but the code there raises on the first exception, which is the same as not handling it.
There was a problem hiding this comment.
Aaah sorry, didn't see it was there twice
| if room_is_empty or last_ex: | ||
| if last_ex: | ||
| raise last_ex | ||
| gevent.sleep(retry_interval) |
There was a problem hiding this comment.
Even if not going for a separate function, I'd still suggest to use stop_event.wait instead of gevent.sleep and breaking early in case it returns True (was set), so it properly return in case client is stopping, instead of lying around way longer than any Runnable's child is supposed to
There was a problem hiding this comment.
Done, changed it _handle_invite as well.
err508
force-pushed
the
#3588
branch
2 times, most recently
from
aea11b9 to
e2be88e
Compare
| last_ex = e | ||
| room_is_empty = not bool(peer_ids & member_ids) | ||
| if room_is_empty or last_ex: | ||
| self._stop_event.wait(retry_interval) |
There was a problem hiding this comment.
You need also to break if return of wait is True, for the loop to exit.
err508
force-pushed
the
#3588
branch
3 times, most recently
from
467a9df to
4207b7c
Compare
| else: | ||
| # Inform the client, that currently no one listens: | ||
| self.log.error( | ||
| 'Peer has not joined from invite yet, should join in the eventually', |
| len(received_messages1) == number_of_received_messages1 + 1 | ||
| ) | ||
| gevent.sleep(.1) | ||
| message = Processed(message_identifier=number_of_received_messages1) |
|
in conftest.py
condition: - Client A invites - The invite triggers _handle_invite in Client B's transport - Client A starts sending messages to Client B - Messages are lost, as the invite was not processed yet The race condition will be fixed in another PR. Appeared during raiden-network#3124, related raiden-network#2779, raiden-network#3123.
if someone joins the room we created. If no peer joins, we leave the room as well. This fixes a race condition, where we sent messages to a room, where no peer has joined yet or never will.
to speed up the sync if necessary. [skip tests]
Adds a check with retries to the
_get_room_for_addressmethod, to prevent sending messages to empty rooms. After discussion with @andrevmatos, we will not leave the room but assume the peer will join eventually.Fixes #3588 race condition, where we sent messages to a room, where no peer has joined yet or never will.