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
