Hi Jim,
Jim Redman wrote:
> Thanks for the fix, there is one problem with the fix and some other
> problems.
>
> I think that the fix needs an else:
>
> // if we get an exception while sending the request,
> // and the connection is a keepalive connection,
> it may
> // have been timed out by the server. Try again.
> if (client.keepalive) {
> client.closeConnection ();
> client.initConnection ();
> in = client.sendRequest (request);
> }
> else { <<<<<<<<<<<------- need to throw here
> if not keep alive.
> throw iox;
> }
>
> Without the else you get all sort of fun null pointers when you don't
> have keepalive.
>
> If keep alive is not set on the client there is another problem.
>
> At or around line 187 is this code.
> // client keepalive is always false if
> XmlRpc.keepalive is false
> if (!client.keepalive)
> client.closeConnection ();
>
> The client needs to be nulled:
>
>
> // client keepalive is always false if
> XmlRpc.keepalive is false
> if (!client.keepalive) {
> client.closeConnection ();
> client = null;
> }
thanks for the fixes, I'm checking them in now.
>
> BUT I think that there is a larger problem that I have no suggested
> fix for. The client.keepalive is a necessary but not a sufficient
> condition. The correct condition is whether the server is honoring
> the keepalive. You only know the answer to this if the server tells
> you, that is, if the server sends a "Connection: close" in the
> response (or a keep-alive, but that works correctly). So if
> client.keepalive is true, but the server does not honor it and returns
> a close, then this code still needs to be executed. The close is
> caught much deeper in the code and so isn't known at this point.
I think the current code handles this correctly already, it's just not
very easy to read. It would be a good idea to make that code easier to
understand and/or document it, and to make sure it really does the right
thing in all situations.
The keepalive field is set two times in HttpClient.sendRequest(), once
when parsing the response status line/protocol version, and once when/if
a Connection: header is parsed.
When reading the status line, the code is:
keepalive = XmlRpc.getKeepAlive() &&
"HTTP/1.1".equals (httpversion);
which means that keepalive is assumed if it is enabled locally and the
response is HTTP version 1.1, which uses persistent connections by
default. For HTTP 1.0 we assume no keep-alive.
Then, when parsing the response headers, there is this code that checks
for a Connection: header:
if (line.startsWith ("connection:"))
keepalive = XmlRpc.getKeepAlive() &&
line.indexOf ("keep-alive") > -1;
so if there is a Connection: header and its value is not "keep-alive",
the keepalive flag is set to false. If there is no Connection: header in
the response, the default value for the minor protocol version is used.
Although I think this code results in the proper behaviour in most
cases, I'd be very interested to hear from people who have better
knowledge of HTTP than me.
Hannes
