[mosh-devel] Questions for agent fwd implementation

[Keith Winstein]

Hi Timo,

Thanks a zillion for your work on this!! As you know this has been a
much-requested feature.

On Tue, May 7, 2013 at 2:27 PM, Timo J. Rinne <tri at iki.fi> wrote:
> 1) Is the entire transport protocol packet format described in
> protobufs/transportinstruction.proto and does that cover the packet
> format for both directions?

Yes, that covers the header for instructions sent in both directions.
The actual diffs (between states of each type) go in the "diff" field.

> 2a) If it is so, would it be possible just add
>     optional uint64 agent_id = 8;
>     optional bytes agent_data = 9;
> to the Instruction definition in order to be able to carry agent
> connection id and actual request (or reply) payload (does not need to be
> a full agent operation packet, any size fragment will do) over the
> transport protocol along with other traffic?

Yess... the issue (if we make the change here) is that you are going
to have to deal with retransmissions and reassembling missing chunks
yourself. Because any particular TransportInstruction can get lost,
and we don't necessarily retransmit the same TransportInstruction
later. E.g. if we sent a TransportInstruction with a diff from state
#3 to state #7, and it gets lost, the next TransportInstruction might
well go from state #3 to state #8. And we have to retransmit the
unacknowledged chunk of agent data. And you have to worry about
getting the data in-order.

I'm wary of breaking the idempotency of the TransportInstruction,
which is basically what we're talking about here.

> 2b) If it isn't so, where should that stuff be added?

I need to put some thought into this. I'm not sure. I think it may be
a good idea for us to develop a more general "stream" abstraction,
perhaps as part of the communicated objects (Terminal::Complete and
Network::UserStream), so we get stuff like retransmissions for free.

> 3) Would the adding of these two optional fields be backwards compatible
> in a sense that the protocol remains identical to the current one for as
> long as there are no agent_id/agent_data sent in any of the packets?
> That would be the case if the client doesn't request agent forwarding or
> if the server doesn't support it even if it's requested?

Yes. protobufs is backwards-compatible even if you do use the fields
-- the old receiver will just ignore the new fields.

> 4a) Should the transport packet format be amended, how can I get the
> agent_id/agent_data pairs in the order they arrive from the transport
> protocol in mosh-server.cc? I see the packets read is initiated in the
> main loop of serve() function like network.recv(). If the packets
> received, contain agent_id/agent_data, I should be able to detect those
> in loop for ( size_t i = 0; i < us.size(); i++ ) where the terminal
> payload is gathered and resizes are detected etc. If I can somehow
> detect that like terminal resizes are detected and retrieve
> agent_id/agent_data, I can immediately handle those within the loop.
> Please help!

If we did it this way, we'd have to amend the Network::Transport
interface to allow this. Right now there is no way for mosh-server.cc
(which instantiates the Network::Transport) to peer inside the details
of a TransportInstruction that is received. It only gets to see the
encapsulated objects made by the diffs (e.g. the UserStream). But (see
above) I think putting this data in a different place is probably
best.

> 4b) The same applies to the client side, where the main loop is in
> STMClient::main(). The structure of the main loop there is a bit
> different, but I'd pretty much need to achieve the same thing, i.e.
> detect the packets that have agent_id/agent_data and handle that
> immediately. Please help!

Ditto.

> 5) When I get data over the agent socket (either the ones created via
> proxy listener in server end or the local agent socket connected by the
> client) I need to be able to incorporate that data to the transport
> packet that is going to be sent over the connection to the other end.
> This would simply mean that I must be able to set agent_id/agent_data
> pair to the outgoing packet in the serve() in server end and
> STMClient::main() in client end. Please help!

Yeah, again the trick here is that the frontend code (by design) only
interacts with SSP by (a) changing the outgoing state object whenever
it wants and (b) getting changes to its incoming state object whenever
one is available. By "state object," I mean something like
Terminal::Complete (incoming for the client, outgoing for the server)
or Network::UserStream (incoming for the server, outgoing for the
client).

So far we haven't exposed the details of the TransportInstruction (or
even how many retransmissions of a TransportInstruction are required
to actually convey a new state) to the caller. I'm not totally eager
to break this abstraction and tend to think it may be better just to
annotate the state objects themselves.

Let me put some thought into what I think is the right protocol change
here and get back to you.

Thanks again,
Keith