On 15.02.2012 05:24, Alex Rousskov wrote:
On 02/14/2012 02:09 AM, Amos Jeffries wrote:
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...
The implementation is different because my earlier idea of tying the
"avoidPconn" state to serverDestinations manipulations does not work
for
pinned connections. However, I am still solving the same problem,
using
essentially the same flag. It is just placed differently so that both
FwdState and pinned connections can share it.
My bugs that you have found below (thank you!) may have clouded the
intent though.
Thought so :P. Audit assumed that was the aim.
* FwdState::startConnectionOrFail() pops one of those N pconn off
the
idle list instead of opening a new connection.
Yes, this is a bug. Will fix by changing the last pop() argument to
the
equivalent of:
!wasUsed && checkRetriable()
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.
As discussed earlier, we cannot bypass the pop() call because we want
to
close an old idle connection if we have to create a new one.
In this case an existing one has just closed/failed. So killing one
will be a net negative on the connection count. Seems to just be a waste
of one of those N other connections (without even checking its
usability).
I'm not arguing for/against this, just pointing out the case here is
not as bad as the expanding connection count we argued out earlier.
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.
I do not think a "used to be a pconn" flag will work here because we
need to detect pconn race conditions and not just any connection
reuse.
Pconn race conditions become impossible (and, hence, failures should
not
be retried using the same destination address) after we receive
something from the server and before the connection becomes idle
again.
Server kids are the only ones that know when we have received
something.
In other words, we need a flag that helps us detect failed
connections
that went through the following sequence of "states":
* idle
* eof
The new wasIdle flag keeps the first state, and Server kids clear
that
flag if they receive something from the server because it means the
eof
will not be reached immediately after the idle state. I do not see
how
we can avoid Server kid modifications, but it would be great if we
can,
of course.
Okay. Leaves us a bit more complicated state to remember updating when
making new server-facing components. But not a major issue.
I do not like the wasIdle name because, technically, the connection
was
not idle after we started writing the request to the server, but I
failed to find a better name. Suggestions welcomed!
will have to ponder that. Something about the read being idle maybe?
Amos
