[mosh-devel] Questions for agent fwd implementation

[Timo J. Rinne]

On 20130508 18:11, Keith Winstein wrote:
> Hi Timo,
> 
> Thanks a zillion for your work on this!! As you know this has been a
> much-requested feature.

No problem.  I admit I'm for a large part doing it just for me :).

> 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.

Right.  So I wasn't completely lost :).

>> 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.

Maybe we could go around it by just adding agent_sequence_number to go
with the agent_id/agent_data so that recipient end can detect cases
where datagram is missing or out of sequential order (each unique
agent_id would have independent sequence numbering starting from 0 and
incremented by 1 in each packet sent).  I'd first try a mechanism that
would simply discard the agent connection if it detects sequence number
error.  If it handles 99.9% of the cases, it's enough.  This is totally
different from generic tunneling case in this sense too.

And behold, idempotency is spared.

>> 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.

That would definitely be a bonus, but remember that it doesn't really
help, if it's something that is not going both upstream and downstream.

>> 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.

That's great!

>> 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.

You may be right.  My changes so far don't really lock this down in any
way.  It may mention transport layer in comment, but what it really
means, is that "deliver this data to another side of the connection".

Once this is figured out and implemented, we are more or less done.

One additional feature that would be useful, is the simple method to
detect if protocol send buffer for some reason is bloated and most
likely blocked.  In this case, agent connection that attempts to send
data, can be dropped immediately.  In server side the code already drops
all open agent connections, if the client seems to be out of reach.
Each individual agent connection is also dropped if it's idle for more
than 30 seconds.

>> 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).

I think solution to this comes directly from the solution to points
4[ab].  I'll just leave it to you :).

> 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.

Now when you put it that way, I think it might be a good idea _not_ to
expose transport level data to the upper layer.  One out of the blue
idea of course might be allowing transport level code to directly access
agent connection object and write data there since it's actually done
without help of the poll-loop anyways.  I'd still rather not do that,
because it would mean spreading agent code in so many places throughout
the code.  I'd rather keep it in client and server main loop, agent
object library and protocol extensions that are strictly necessary.


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

Great!

> Thanks again,
> Keith
> 

Regards,
//Rinne