The idea behind distinguishing between the user being connected and
the actual connection having been made was to support retries while
the user decided to connect, even if the connection wasn't made yet.
The problem is that _user_connected is used directly to tell whether
the sender is connected or not which will be wrong if an exception
occurs on the initial connection. Since the logic retry isn't used
there we can simply mark as connected once a successfull connection
is made.
This prevents us from locking forever on any task that doesn't
rely on cancellation tokens, in this case, Connection.recv()'s
_recv_queue.get() would never complete after the server closed
the connection.
Additionally, working with cancellation tokens in asyncio is
somewhat annoying to do.
Last but not least removing the Connection._disconnected future
avoids the need to use its state (if an exception was set it
should be retrieved) to prevent asyncio from complaining, which
it was before.
This issue would likely be triggered when automatically
merging multiple requests into a single one while having
their size exceed 1044456 bytes like SaveFilePartRequest.
This commit avoids such issue by keeping track of the
current size, and if it exceeds the limit, avoid merge.
This has several benefits:
- The message can be resent without re-calling bytes(),
which for some requests may be expensive.
- Converting requests to bytes early lets us detect
errors early, such as OverflowError on bad requests.
- Containers can't exceed 1044456 bytes so knowing their
length is important. This can now be done in O(1).
But also several drawbacks:
- If the object is modified the bytes won't reflect this.
This isn't an issue because it's only done for in msgs.
- Incoming messages can no longer be reconverted into
bytes but this was never needed anyway.
