Conversation
robertbartel
force-pushed
the
f/client_updates/mux_client_refactor
branch
2 times, most recently
from
8a5ac6c to
abbd499
Compare
Adding to TransportClientMultiplexer as multiplexing wrapper that can spawn specialized MuxStreamClient subtype of TransportLayerClient.
robertbartel
force-pushed
the
f/client_updates/mux_client_refactor
branch
from
abbd499 to
abf57aa
Compare
aaraney
left a comment
aaraney
left a comment
There was a problem hiding this comment.
The implementation looks solid, @robertbartel. Unless I am missing something, it seems that the request service and likely all other services will need to be modified to accommodate handling stream ordering if we want to use a TransportClientMultiplexer to communicate with them, right? If that is the case, we should probably throw an exception when trying to create a TransportClientMultiplexer until those changes have been introduced. Thoughts?
| if len(mux_id) != self._len_mux_id: | ||
| msg = f"{self.__class__.__name__} can't send using mux id {mux_id} (expected length {self._len_mux_id!s})" | ||
| raise ValueError(msg) | ||
| await self._wrapped_client.async_send(f"{mux_id}{data}") |
There was a problem hiding this comment.
It seems that we will need to make changes to the request service's deserialization and serialization behavior to accommodate this change, right? One fear that I have is that this custom serialization format will spread across the entire codebase. For example, if you wanted to use this client and communicate with a particular service directly, that service would have to implement the custom deserialization format and handle stream ordering too.
|
You are correct, @aaraney. I got a little too focused on the looking glass and forgot about what was on the other side of it. For now, I think we leave this as Draft/Blocked. This is going to need a proper server-side solution to go along with it. I'm not exactly sure what form that will need to take yet, and it may inform changes to what I initially had here. |
Okay, cool! Yeah, im not sure either. Ill spend some time thinking about what that might look like too and maybe we can come together and compare our thoughts. |
2 participants
Adding
TransportClientMultiplexertype to wrap aTransportLayerClientinstance in order to share the latter's connection, along with specializedMuxStreamClientconcrete type that implementsTransportLayerClientand is spawned from the wrapper to share the wrapped client.This addresses the problem of users of a
TransportLayerClientinstance relying upon ordering of incoming messages in a way that is not guaranteed when there are multiple users.This PR relies on changes in #409 and should remain in draft status until that has been merged.