Prevent possibly unbound variable in history response handler

[phijor]

When handling history responses, the response future in future might be unbound in the HistoryError handler.

Split this in three steps to reduce error handling clutter:

1. Obtain future from correlation ID, which is possibly unknown
2. Check that this future is not already done, i.e. the response was not handled already
3. Parse history response, set result or exception of the response future accordingly

[tilsche]

I think I understand what you are trying to do, but I don't understand the comment.

• What does it mean that "the future in future is unbound"?
• Is this something that happened to you, or did you determine this an issue through the method of staring at the code?
• How does the handling of HistoryError have any say here? Do you men the internal handler which just does set_exception rather than set_result? Or the "outside" Handler in user code, that should happen well after any of this?

I can only see the critical case happening when a client receives two successive messages with the same correlation id - which would be a bug in another agent, but sure, could happen - and the second _on_history_response gets handled by the loop before asyncio.wait_forself._request_futures[...] completes leading to the removal of the future from the map.

[phijor]

* What does it mean that "the future in `future` is unbound"?

Unfortunate wording. The local variable future is only set in the try: ... block, so if we try to use it in an except ...: ...-clause, python might raise an UnboundLocalError.
Sure, right now there is no except ...:-clause that references future before it was set, but that's not easy to see.

* Is this something that happened to you, or did you determine this an issue through the method of staring at the code?

Staring at the code and having pyright complain about it.

* How does the handling of `HistoryError` have any say here? Do you men the internal handler which just does `set_exception` rather than `set_result`? Or the "outside" Handler in user code, that should happen well after any of this?

If I understand correctly what you are asking:
I am talking about the internal error handler which just does future.set_result.
In my mind the single error handler was just to complicated, it tried to handle three different kinds of (unrelated) error conditions. And for me it was not immediatly clear what line raises what exception, so I split it up.

I can only see the critical case happening when a client receives two successive messages with the same correlation id - which would be a bug in another agent, but sure, could happen - and the second _on_history_response gets handled by the loop before asyncio.wait_forself._request_futures[...] completes leading to the removal of the future from the map.

Yes, that is (probably) correct. It was just that the handling for "this is an unknown correlation ID" and "this response was handled already" were lumped together. I think that if a response future can be retrieved from self._request_futures it is actually never done, but I did not want to remove the code handling this condition.

I am not sure if the code under except (..., asyncio.InvalidStateError) ever ran in practice.

[tilsche]

I

* What does it mean that "the future in `future` is unbound"?

Unfortunate wording. The local variable future is only set in the try: ... block, so if we try to use it in an except ...: ...-clause, python might raise an UnboundLocalError.
Sure, right now there is no except ...:-clause that references future before it was set, but that's not easy to see.

* Is this something that happened to you, or did you determine this an issue through the method of staring at the code?

Staring at the code and having pyright complain about it.

I'd argue it can't happen because self._request_futures[correlation_id] will never raise a HistoryError. But sure, it's more obvious that way.

I am not sure if the code under except (..., asyncio.InvalidStateError) ever ran in practice.

Ah I skimped over the asyncio.InvalidStateError now being replaced by the apocalyptic future.done() check.