On Thu, Aug 21, 2014 at 5:47 PM, Justin Erenkrantz <jus...@erenkrantz.com> wrote: > On Thu, Aug 21, 2014 at 5:30 PM, Lieven Govaerts <l...@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
