search

shell-pool / shpool

Think tmux, then aim... lower
Apache License 2.0
1.17k stars 20 forks source link

Shpool's internal protocol is not very forward compatible #85

Closed ethanpailes closed 4 months ago

ethanpailes commented 4 months ago

Currently, shpool uses serde for serialization derevation and bincode for our actual codec. We need this in order to serialize messages that are sent between the attach process and daemon process. The problem is that bincode seems to have been developed primarily with speed in mind, and not with compatibility between types with slight skew in mind. This is bad because it means every time we touch the protocol, people can no longer talk to their daemon with their new client binary and must restart the daemon. The shpool daemon is expected to run for a while, and losing access to your sessions after an update is sad.

For proper forwards compatibility, a codec must do two things

  1. be able to read messages containing unknown fields, ignoring any of these unknown fields
  2. be able to read messages containing missing fields, setting zero/default values for these fields (half points for only supporting Option here, but really that's too clunky)

Protobuf does this correctly, but I would like to stay away from protobuf because I have not heard great things about current rust protobuf oss offerings and if possible it would be nice to wait until a google version option gets open sourced, which I anticipate will have better APIs than existing oss options. Also, I just should not need to bring out a codegenerator to solve this issue.

codec options

I have not been able to find a serde codec that does this properly yet. I'll take some notes here to try to find one that does. I'm pretty worried that this might just be impossible because rust is just not very friendly to (2) and most codec implementors seem focused on speed more the boring engineering stuff like forwards compatibility.

bincode

As far as I can tell bincode is a complete lost cause.

rmp-serde

rmp-serde (the messagepack codec serde adapter) seem to be a bit better. It has a .with_struct_map() option you can set when serializing which will fulfill (1), but it does nothing to help us with (2).

serde_json

out of the box it can handle (1), but not (2)

ethanpailes commented 4 months ago

There seems to be a #[serde(default)] annotation that should take care of (2) for us, so rmp-serde should work after all so long as we annotate all the fields in the protocol file with default. It would be nice to be able to set the option globally, or at least per-struct.