[ovs-dev] [PATCH 2/3] jsonrpc: Treat draining data from send queue as activity.

[Ansis Atteka]

On Thu, Sep 6, 2012 at 7:49 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Sep 05, 2012 at 11:48:26PM -0700, Ansis Atteka wrote:
>> On Wed, Aug 8, 2012 at 4:14 PM, Ben Pfaff <blp at nicira.com> wrote:
>> > Until now, the jsonrpc module has used messages received from the
>> > remote peer as the sole means to determine that the JSON-RPC
>> > connection is up.  This could in theory interact badly with a
>> > remote peer that stops reading and processing messages from the
>> > receive queue when there is a backlog in the send queue for a
>> > given connection (ovsdb-server is an example of a program that
>> > behaves this way).  This commit fixes the problem by expanding
>> > the definition of "activity" to include successfully sending
>> > JSON-RPC data that was previously queued.
>> >
>> > The above change is exactly analogous to the similar change
>> > made to the rconn library in commit 133f2dc95454 (rconn: Treat
>> > draining a message from the send queue as activity.).
>> >
>> > Bug #12789.
>> > Signed-off-by: Ben Pfaff <blp at nicira.com>
>> > ---
>> >  lib/jsonrpc.c         |   13 +++++++++++++
>> >  python/ovs/jsonrpc.py |   11 ++++++++++-
>> >  2 files changed, 23 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
>> > index c4d7dd2..9c0aaa9 100644
>> > --- a/lib/jsonrpc.c
>> > +++ b/lib/jsonrpc.c
>> > @@ -880,9 +880,22 @@ jsonrpc_session_run(struct jsonrpc_session *s)
>> >      }
>> >
>> >      if (s->rpc) {
>> > +        size_t backlog;
>> >          int error;
>> >
>> > +        backlog = jsonrpc_get_backlog(s->rpc);
>> >          jsonrpc_run(s->rpc);
>> > +        if (jsonrpc_get_backlog(s->rpc) < backlog) {
>> > +            /* Data previously caught in a queue was successfully sent.
>> Is this comment accurate that "data was sent successfully"? I suppose
>> that, if jsonrpc_get_backlog(s->rpc) returned 0, then that might mean
>> an error in jsonrpc_run(s->rpc) too?
>
> Yes, that's also a possibility.  In that case, we'll catch the error
> and disconnect in the jsonrpc_get_status() test just below:
>
>> > +             *
>> > +             * We don't count data that is successfully sent immediately as
>> > +             * activity, because there's a lot of queuing downstream from us,
>> > +             * which means that we can push a lot of data into a connection
>> > +             * that has stalled and won't ever recover.
>> > +             */
>> > +            reconnect_activity(s->reconnect, time_msec());
>> > +        }
>> > +
>> >          error = jsonrpc_get_status(s->rpc);
>> >          if (error) {
>> >              reconnect_disconnected(s->reconnect, time_msec(), error);
>
> If you like, I'll make the comment mention that case.
Thanks! Looks good to me.