De-generalize Connection::recv

[vilcans]

When I tried to integrate Crystalorb with custom networking, I found that it's not as simple as it can be.

The function Connection::recv is declared as follows:

    /// Interface for CrystalOrb to request the next message received, if there is one.
    ///
    /// CrystalOrb will invoke this method with the three message types as specified in
    /// [`NetworkResource`].
    fn recv<MessageType>(&mut self) -> Option<MessageType>
    where
        MessageType: Debug + Clone + Serialize + DeserializeOwned + Send + Sync + 'static;

It is generic on the MessageType, but it actually only supports three messages, as the documentation says. Having it generic on MessageType is as far as I can tell an unnecessary complication. I found it hard to follow the code in crystalorb_mock_network because of the code required to manage this.

If I check the callers of the recv function in the crystalorb code base, it always looks like one of the following statements:

connection.recv::<Timestamped<WorldType::CommandType>>()
connection.recv::<Timestamped<WorldType::SnapshotType>>()
connection.recv::<ClockSyncMessage>()

I.e. the type of the message is known at the call-site and does not have to be generic.

Wouldn't it be better if Connection instead declared those specific messages? I.e. something like this:

fn recv_command(&mut self) -> Option<Timestamped<WorldType::CommandType>>;
fn recv_snapshot(&mut self) -> Option<Timestamped<WorldType::SnapshotType>>;
fn recv_clock_sync(&mut self) -> Option<ClockSyncMessage>;

Then the code would not have to rely on run-time type information (TypeId) and would be more straight-forward.

Let me know what you think. I may have missed the reasoning behind the design.

[ErnWong]

I totally agree. Not only would that make things easier to integrate with custom networking, but hopefully that'll also make it easier to see what messages get sent (EDIT: and received) (without relying on documentation (which could get out of date), and without digging into the codebase).

Thanks for raising this!

[vilcans]

I tried changing this, but when it came to sending messages, I thought it was more convenient to have an enum like:

#[derive(Clone, Debug)]
pub enum Message<CommandType, SnapshotType>
where
    SnapshotType: Debug + Clone + Serialize + DeserializeOwned + Send + Sync + 'static,
{
    ClockSync(ClockSyncMessage),
    Snapshot(Timestamped<SnapshotType>),
    Command(Timestamped<CommandType>),
}

Then only one send function is needed:

    fn send(
        &mut self,
        message: Message<WorldType::CommandType, WorldType::SnapshotType>,
    ) -> Option<Message<WorldType::CommandType, WorldType::SnapshotType>>;

...but that would be inconsistent with recv being three functions.

recv could be only one function if it can return any message type:

    fn recv(&mut self) -> Option<Message<WorldType::CommandType, WorldType::SnapshotType>>;

...but the code calling recv is implemented to only receive messages of a specific type, so that won't work.

I'll make a public repo with the changes.

[ErnWong]

I'll make a public repo with the changes.

Thanks! That'll be great.

but that would be inconsistent with recv being three functions.

I probably don't mind this inconsistency of having three recv functions but one send function.

Yeah, currently CrystalOrb assumes that it can pull out one type of message without pulling out other types of messages. For network implementations that demultiplex the messages into separate queues, this works out nicely. For network implementations where all messages end up in the same queue, you'd probably have to demultiplex the messages into separate queues yourselves.

Regarding whether or not CrystalOrb should internally handle the demultiplexing (to further simplify the network resource interface) or not, I think CrystalOrb is probably better as it currently is (i.e. leaving the demultiplexing to the networking implementation) so that we don't need to unnecessarily multiplex and demultiplex twice for network implementations that already handle demultiplexing for us.

[ErnWong]

Done via #9