fix(client): Fix cleanup of Client._waiters dict

[qweeze]

fixes #125

[DanielePalaia]

Thanks!

[garvenlee]

In addition, function wait_frame also causes this bug.

[qweeze]

In addition, function wait_frame also causes this bug.

Do you mean creating a set in self._waiters[_key].add(fut) ? But this is intentional - this keys will either be deleted from _waiters in _listener, or the future will time out, and an exception will be raised.

[garvenlee]

If the waiter is timeout, its callback will discard itself from self._waiters[key], but the underlaying set still exists?

And I don’t understand why self._waiters has to be a defautdict(set)? Each frame has a unique correlation_id, so the waiter_key is also unique.

[qweeze]

but the underlaying set still exists?

Yes, but I didn't consider it a big issue because it only happens when the app crashes, and recovery would require re-instantiating Client anyway, so at this point we don't care about memory. But I see your point now, and I agree it's better to not to leave any dangling references anyway

Each frame has a unique correlation_id, so the waiter_key is also unique

According to protocol, one-way messages (commands, i.e. Deliver) don't have correlation_id. So basically storing waiters in a set allows having multiple concurrent calls to wait_frame for a single command.

But as I can see, currently this is not used in the project. So I think we can simplify this structure, get rid of sets altogether and make it a plain dict[key, Future] But only if we're sure that we won't need this "multiple waiters" functionality - perhaps @Gsantomaggio and @DanielePalaia can confirm that is redundant

[garvenlee]

it only happens when the app crashes, and recovery would require re-instantiating Client anyway

Yes, it is.

storing waiters in a set allows having multiple concurrent calls to wait_frame

Note that _waiters is only used in sync_request, where corr_id_seq is used to guarantee the uniqueness. This supports concurrency with different corr_id, so it’s fine to make _waiters a plain dict, and it’s also possible to simplify _handlers as here.

[qweeze]

Ok, made waiters a plain dict

it’s also possible to simplify _handlers

I suggest to work on this in a next PR

[qweeze]

@Gsantomaggio do you mind if I merge this?

[Gsantomaggio]

it is ok with me! thank you @qweeze