connection: release request ids after failed sends#874
connection: release request ids after failed sends#874dkropachev wants to merge 3 commits intoscylladb:masterfrom
Conversation
dkropachev
force-pushed
the
dk/connection-send-failure-cleanup
branch
from
32e8008 to
856efaa
Compare
| @@ -1262,9 +1266,16 @@ def wait_for_responses(self, *msgs, **kwargs): | |||
| self.in_flight += available | |||
|
|
|||
| for i, request_id in enumerate(request_ids): | |||
| self.send_msg(msgs[messages_sent + i], | |||
| request_id, | |||
| partial(waiter.got_response, index=messages_sent + i)) | |||
| try: | |||
| self.send_msg(msgs[messages_sent + i], | |||
| request_id, | |||
| partial(waiter.got_response, index=messages_sent + i)) | |||
| except Exception: | |||
| unsent_request_ids = request_ids[i:] | |||
| with self.lock: | |||
| self.in_flight -= len(unsent_request_ids) | |||
| self.request_ids.extend(unsent_request_ids) | |||
| raise | |||
| messages_sent += available | |||
There was a problem hiding this comment.
There are now multiple PRs regarding request_ids, in_flight etc.
It is incredible that we need to ever worry about this stuff.
Why is it even responsibility of the caller to adjust those values?
Connection should have a method for sending request. This method should be responsible for managing in_flight, request_ids and other state of Connection. Callers should never worry about that.
This is the only sane solution, and anything else will just require fixing callsites forever.
There was a problem hiding this comment.
And yes I know this is a code in connection. But you also have PRs for e.g. hearbeats. Heartbeats should never need to touch this stuff.
There was a problem hiding this comment.
Acknowledged. I removed the async keyspace cleanup from this branch, so this PR is now scoped to the concrete send-failure leak only. The broader connection-level helper/refactor can stay as a separate follow-up.
|
As I said on the hearbeat PR - maybe the |
2 participants
Fixes #873. Reclaims request ids when send_msg fails and covers the async keyspace path with unit tests.