Re: [PREVIEW] Retry requests that failed due to a pconn race

On 14/02/2012 7:03 p.m., Alex Rousskov wrote:
> Hello,
>
> This has been discussed before (see the old thread quoted below),
> and the bump-ssl-server-first feature is being bitten by this especially
> badly.
>
> The attached patch may not apply to trunk "as is" right now, but the
> changes should be clearly visible, and I would like to port it there and
> get it committed if there are no better ideas for the fix.
>
> Technical summary:
>
> The ERR_ZERO_SIZE_OBJECT errors were visible to the client when the
> destination had only one address because serverDestinations.shift()
> made the list of destination empty and startConnectionOrFail() failed.
>
> The new "wasIdle" state is remembered in the Connection object instead
> of FwdState itself because we need to maintain that information for a
> pinned connection as well (and FwdState is not preserved between a
> request that pins a connection and a request that uses the pinned
> connection).
>
> This may need more work. I am not sure the FTP and Gopher cases are
> correct. The current code assumes that those protocols do not have pconn
> [races].
>

Um. This patch is not doing what I thought you were arguing for it to do...

* You set it on IdleConnList::push, but missed its twin
ConnStateData::pinConnection() in client_side.cc

* FwdState::startConnectionOrFail() pops one of those N pconn off the
idle list instead of opening a new connection. So you implemented what I
was ask
  You need to wrap that pop() call using
if(serverDestinations[0]->wasIdle) in order to get a brand new
connection on retries.

Also, I think this flag would work better if it were a generic flag to
say the FD was a pconn/pinned in the past. Making TcpOpener the only
place setting it to false, as a brand new connection is attached to its
FD. The ServerData child components dont need to even know about it or
assume anything about races.

Amos
Received on Tue Feb 14 2012 - 09:09:54 MST