Re: [serf-dev] Re: [PATCH] Add a configuration option to disable HTTP pipelining.

[Justin Erenkrantz <ju...@erenkrantz.com>]

On Thu, Aug 21, 2014 at 5:30 PM, Lieven Govaerts <lg...@mobsol.be> wrote:
> Attached patch implements a slightly different approach but I think
> it's a valid workaround for the problem.

It may be useful to note why we register apps_ssl_info_callback in the
re-negotiate info callback.  (It's because there can only be one
callback.)

I'd also like to confirm whether we always re-init the SSL context
when we reset the connection - I think so, but didn't confirm...

> What it does is detect when a server triggers a renegotiation, and
> when that happens switch from a pipelined to a non-pipelined
> connection. It doesn't check if the application is actually using the
> pipelining, just if it's enabled or not.

As we discussed, this is the only thing that bothers me a bit - any
time we see a renegotiation, we'll reset the connection and try again
with pipelining turned off.  I think we could do something to factor
in the state of serf's connection state - that is, something like
(conn->completed_requests - conn->completed_responses > 1) to not
reset the connection when there is only just the one pending response
that triggers the renegotiation.  Ostensibly, after the renegotiation
succeeds, we *could* do pipelining again - we just can't handle
renegotiation when there is more than one outstanding request pending.

> I'm not convinced this completely removes the need for the
> "http-pipelining" option, because I'm not convinced this will cover
> all possible situations, but it should at least cover the ones
> reported to the serf dev list.

Agreed.  But, I'm tempted to apply this patch and then see if we still
get real-world complaints before adding yet-another-knob.  -- justin

Re: [serf-dev] Re: [PATCH] Add a configuration option to disable HTTP pipelining.

[Lieven Govaerts <lg...@mobsol.be>]

On Thu, Aug 21, 2014 at 5:47 PM, Justin Erenkrantz
<ju...@erenkrantz.com> wrote:
> On Thu, Aug 21, 2014 at 5:30 PM, Lieven Govaerts <lg...@mobsol.be> wrote:
>> Attached patch implements a slightly different approach but I think
>> it's a valid workaround for the problem.
>
> It may be useful to note why we register apps_ssl_info_callback in the
> re-negotiate info callback.  (It's because there can only be one
> callback.)

Fixed.

> I'd also like to confirm whether we always re-init the SSL context
> when we reset the connection - I think so, but didn't confirm...

Management of the lifecycle the ssl context is handled by the
application, not by serf. The SSL context is created when the
encrypt/decrypt buckets are created, which is done by the application
in the connection_setup handler. By just looking at the
connection_setup handler it seems at first that Subversion is  reusing
the ssl context, but it will reset it to NULL in the connection_closed
handler, so that should be ok.

>
>> What it does is detect when a server triggers a renegotiation, and
>> when that happens switch from a pipelined to a non-pipelined
>> connection. It doesn't check if the application is actually using the
>> pipelining, just if it's enabled or not.
>
> As we discussed, this is the only thing that bothers me a bit - any
> time we see a renegotiation, we'll reset the connection and try again
> with pipelining turned off.  I think we could do something to factor
> in the state of serf's connection state - that is, something like
> (conn->completed_requests - conn->completed_responses > 1) to not
> reset the connection when there is only just the one pending response
> that triggers the renegotiation.  Ostensibly, after the renegotiation
> succeeds, we *could* do pipelining again - we just can't handle
> renegotiation when there is more than one outstanding request pending.

I propose we implement this after we switch to the new
protocol/connection implementation. The separation between context and
buckets makes it very difficult to find out how many outstanding
requests there are on a connection from within the BIO read/write
code.

>> I'm not convinced this completely removes the need for the
>> "http-pipelining" option, because I'm not convinced this will cover
>> all possible situations, but it should at least cover the ones
>> reported to the serf dev list.
>
> Agreed.  But, I'm tempted to apply this patch and then see if we still
> get real-world complaints before adding yet-another-knob.  -- justin
>

Fine by me.

Lieven