[mosh-devel] Questions for agent fwd implementation

[Timo J. Rinne]

Hi

How about if I isolate agent like this:

namespace Agent {
  class AgentServer {
  public:
    AgentServer(bool dummy = false);
    ~AgentServer();
    void pre_poll();
    void post_poll();
    void input(uint64_t agent_seq_no, uint64_t agent_id, std::string
agent_data);
    bool output(uint64_t &agent_seq_no, uint64_t &agent_id, std::string
&agent_data, size_t max_agent_data_len = 0);
    std::string listener_path();
    void close_sessions();
    void shutdown_server();
  private:
    ...
  }

Used as follows:

- before main loop calls poll, it calls as.pre_poll();
- at some point after it has waken up from poll, main loop calls
as.post_poll();
- at any point in the protocol code, where incoming agent traffic from
the client is detected, as.input(...) is called with the data.
- at any point the protocol code is ready to send some agent stuff to
the client, it can call as.output(...) to retrieve a piece of output to
be sent. size of the single chunk can be limited and call can be
repeated for as long as all data is sent.

In the client side, everything is basically the same.

//Rinne


On 20130510 10:28, Timo J. Rinne wrote:
> Hi Keith
> 
> Now I think I figured out better way to do that.  Maybe I should make
> the agent functionality hide in one object that is seen by the client
> and server main loops (unlike now when main loops also see each
> individual agent connection).  The object can export file descriptors to
> the fd set for the polling and then it just handles the input from local
> sockets when called after poll.  In addition to that, protocol layer
> will see the object and is able to push any
> agent_seqno/agent_id/agent_data triplet to agent object when it receives
> such data and the agent object handles it accordingly.  Similarly when
> sending packets to the network, protocol layer can ask from the agent
> object, whether it has some agent_seqno/agent_id/agent_data triplets to
> send to the other end, and if there is, the protocol layer will fetch
> them from the agent object and send them.  The agent object can also
> handle the reordering of the incoming packets (up to some extent) quite
> easily.
> 
> Would this ease the pain?
> 
> Regards,
> /Rinne
> 
> On 20130508 19:46, Timo J. Rinne wrote:
>> 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
>>
>