You are viewing a plain text version of this content. The canonical link for it is here.
Posted to rpc-dev@xml.apache.org by ha...@apache.org on 2002/02/27 19:29:19 UTC
cvs commit: xml-rpc/src/java/org/apache/xmlrpc XmlRpcClientLite.java
hannes 02/02/27 10:29:19
Modified: src/java/org/apache/xmlrpc XmlRpcClientLite.java
Log:
Fixed a bug with keepalive connections reported by Jim Redman <ji...@ergotech.com>
A timed out keepalive connection wasn't detected because the exception was thrown
not when trying to send the request to the server but only when trying to parse
the server's response. The fix consists in taking the retry code out of the HttpClient.write()
method and move it up into LiteWorker.execute(). While doing this I also refactored the
HttpClient.write() and HttpClient.getInputStream() methods into one single
HttpClient.sendRequest() method.
Revision Changes Path
1.4 +29 -39 xml-rpc/src/java/org/apache/xmlrpc/XmlRpcClientLite.java
Index: XmlRpcClientLite.java
===================================================================
RCS file: /home/cvs/xml-rpc/src/java/org/apache/xmlrpc/XmlRpcClientLite.java,v
retrieving revision 1.3
retrieving revision 1.4
diff -u -r1.3 -r1.4
--- XmlRpcClientLite.java 18 Feb 2002 23:22:29 -0000 1.3
+++ XmlRpcClientLite.java 27 Feb 2002 18:29:19 -0000 1.4
@@ -157,9 +157,22 @@
if (client == null)
client = new HttpClient (url);
- client.write (request);
+ InputStream in = null;
- InputStream in = client.getInputStream ();
+ // send request to the server and get an input stream
+ // from which to read the response
+ try {
+ in = client.sendRequest (request);
+ } catch (IOException iox) {
+ // 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);
+ }
+ }
// parse the response
parse (in);
@@ -238,7 +251,6 @@
BufferedOutputStream output;
BufferedInputStream input;
boolean keepalive;
- boolean fresh;
public HttpClient (URL url) throws IOException
@@ -256,7 +268,6 @@
protected void initConnection () throws IOException
{
- fresh = true;
socket = new Socket (hostname, port);
output = new BufferedOutputStream (socket.getOutputStream());
input = new BufferedInputStream (socket.getInputStream ());
@@ -272,46 +283,25 @@
{}
}
- public void write (byte[] request) throws IOException
+ public InputStream sendRequest (byte[] request) throws IOException
{
- try
- {
- output.write (("POST "+uri + " HTTP/1.0\r\n").getBytes());
- output.write ( ("User-Agent: "+XmlRpc.version +
+ output.write (("POST "+uri + " HTTP/1.0\r\n").getBytes());
+ output.write ( ("User-Agent: "+XmlRpc.version +
"\r\n").getBytes());
- output.write (("Host: "+host + "\r\n").getBytes());
- if (XmlRpc.getKeepAlive())
- output.write ("Connection: Keep-Alive\r\n".getBytes());
- output.write ("Content-Type: text/xml\r\n".getBytes());
- if (auth != null)
- output.write ( ("Authorization: Basic "+auth +
+ output.write (("Host: "+host + "\r\n").getBytes());
+ if (XmlRpc.getKeepAlive())
+ output.write ("Connection: Keep-Alive\r\n".getBytes());
+ output.write ("Content-Type: text/xml\r\n".getBytes());
+ if (auth != null)
+ output.write ( ("Authorization: Basic "+auth +
"\r\n").getBytes());
- output.write (
+ output.write (
("Content-Length: "+request.length).getBytes());
- output.write ("\r\n\r\n".getBytes());
- output.write (request);
- output.flush ();
- fresh = false;
- }
- catch (IOException iox)
- {
- // if the connection is not "fresh" (unused), the exception may have occurred
- // because the server timed the connection out. Give it another try.
- if (!fresh)
- {
- initConnection ();
- write (request);
- }
- else
- {
- throw (iox);
- }
- }
- }
-
+ output.write ("\r\n\r\n".getBytes());
+ output.write (request);
+ output.flush ();
- public InputStream getInputStream () throws IOException
- {
+ // start reading server response headers
String line = readLine ();
if (XmlRpc.debug)
System.err.println (line);
Re: More keep alive issues...
Posted by Hannes Wallnöfer <ha...@helma.at>.
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
Re: More keep alive issues...
Posted by Hannes Wallnöfer <ha...@helma.at>.
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
More keep alive issues...
Posted by Jim Redman <ji...@ergotech.com>.
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;
}
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.
Jim
--
Jim Redman
(505) 662 5156
http://www.ergotech.com
More keep alive issues...
Posted by Jim Redman <ji...@ergotech.com>.
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;
}
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.
Jim
--
Jim Redman
(505) 662 5156
http://www.ergotech.com