Performance improvements to XMLRPC

This patch is a combination of various improvements to SocketServer, BaseHTTPServer, SimpleXMLRPCServer and xmlrcplib. They are the result of deploying XMLRPC in a production environment at CCP.

There are three parts:
1) Make the XMLRPC client and server handle HTTP/1.1 with keep-alive
2) make the xmlrpc client and server capable of using the gzip content encoding for the xml
3) Implement socket timeout for servers.

I have been tying to submit these patches separately but it is an ardous process.  Perhaps it is better to get this accepted as a single patch?

[krisvale, 2009-06-11 15:25:51]

http://codereview.appspot.com/73041/diff/1/8
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/1/8#newcode1413
Line 1413: connection.putheader("Content-Encoding", "gzipp")
typo, should be "gzip"

http://codereview.appspot.com/73041/diff/1/8#newcode1429
Line 1429: print "gudn"
This will go away

[Benjamin, 2009-06-11 16:24:58]

Do you have tests for the gzip encoding?

http://codereview.appspot.com/73041/diff/1/3
File Lib/BaseHTTPServer.py (right):

http://codereview.appspot.com/73041/diff/1/3#newcode317
Line 317: if not self.parse_request(): # An error code has been sent, just exit
Is this line of 80 characters?

http://codereview.appspot.com/73041/diff/1/3#newcode329
Line 329: self.close_connection=1
The spacing is odd on this line.

http://codereview.appspot.com/73041/diff/1/8
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/1/8#newcode1149
Line 1149: f = StringIO.StringIO()
You used try, finally for closing the files in gzip_decode, so why not here?

http://codereview.appspot.com/73041/diff/1/8#newcode1569
Line 1569: raise AttributeError("method %r not found"%method)
There should be space between the string and % and it's argument. Also, the
argument for % formatting should always be a tuple.

[krisvale, 2009-06-11 21:02:02]

Also, I will add gzip tests

http://codereview.appspot.com/73041/diff/1/3
File Lib/BaseHTTPServer.py (right):

http://codereview.appspot.com/73041/diff/1/3#newcode317
Line 317: if not self.parse_request(): # An error code has been sent, just exit
On 2009/06/11 16:24:58, Benjamin wrote:
> Is this line of 80 characters?
Yes, fixed.

http://codereview.appspot.com/73041/diff/1/3#newcode329
Line 329: self.close_connection=1
On 2009/06/11 16:24:58, Benjamin wrote:
> The spacing is odd on this line.

Done.

http://codereview.appspot.com/73041/diff/1/8
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/1/8#newcode1149
Line 1149: f = StringIO.StringIO()
On 2009/06/11 16:24:58, Benjamin wrote:
> You used try, finally for closing the files in gzip_decode, so why not here?
I'm rather inclined to remove it from gzip_decode, it is rather pointless for
these objects.

http://codereview.appspot.com/73041/diff/1/8#newcode1569
Line 1569: raise AttributeError("method %r not found"%method)
On 2009/06/11 16:24:58, Benjamin wrote:
> There should be space between the string and % and it's argument. Also, the
> argument for % formatting should always be a tuple.

Done.

[josiah.carlson, 2009-06-14 17:47:42]

http://codereview.appspot.com/73041/diff/1/5
File Lib/SimpleXMLRPCServer.py (right):

http://codereview.appspot.com/73041/diff/1/5#newcode435
Line 435: encode_threshold = 1400 #a common MTU
I notice that here you reference MTU, and underlying physical packet size
(ethernet frames are 1500 bytes, Windows default MTU is 1500 bytes for
non-dialup connections, ...).  But in the docs, you reference "TCP packet",
which, technically, can be anything from 1 byte to 64k*.

Using the same language referencing the data actually able to be sent in the
common case (MTU - 44/48 bytes for TCP+IP4/6 headers), would be nice.

[krisvale, 2009-06-14 21:04:19]

Updated the patch.  Added error handling for gzip decode error

[krisvale, 2009-06-14 21:40:44]

Fix unittest.  Move 'disable_nagle_algorithm' to StreamRequestHandler, where it
is more appropriate.

[krisvale, 2009-06-14 21:43:45]

This is a message

http://codereview.appspot.com/73041/diff/5003/6005
File Lib/SocketServer.py (right):

http://codereview.appspot.com/73041/diff/5003/6005#newcode377
Line 377: - disable_nagle_algorithm
This was recently added by Me.  It makes more sense to keep it in the
StreamRequestHandler where wfile and wbufsize are defined.

http://codereview.appspot.com/73041/diff/6010/5012
File Lib/SimpleXMLRPCServer.py (right):

http://codereview.appspot.com/73041/diff/6010/5012#newcode439
Line 439: wbufsize = -1
Make the default server high-performing

[Benjamin, 2009-06-15 03:08:44]

It looks good now. :)

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode1154
Line 1154: f.close()
I agree with you. I think closing StringIO is not really needed.

[Martin v. Löwis, 2009-06-20 16:48:45]

I'm not sure whether the change can go into 2.7, because it causes
incompatibilities.

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (left):

http://codereview.appspot.com/73041/diff/6010/5015#oldcode1264
Line 1264: def send_host(self, connection, host):
I think removal of send_host is an incompatible change.

I understand preserving it would be difficult for this patch, so it may be
better to apply it to 3.x only.

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode52
Line 52: #
You will need to add a history line here.

[krisvale, 2009-06-21 17:21:56]

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (left):

http://codereview.appspot.com/73041/diff/6010/5015#oldcode1264
Line 1264: def send_host(self, connection, host):
On 2009/06/20 16:48:46, Martin v. Löwis wrote:
> I think removal of send_host is an incompatible change.
> 
> I understand preserving it would be difficult for this patch, so it may be
> better to apply it to 3.x only.

The function can be kept, either as a dummy, or if the
HTTPConnection.putrequest() is instructed not to add a "host".  I would prefer
the former, since setting the host as part of the putrequest seems to have been
added deliberately to make sure that the http/1.1 connennection is consistently
created.
I did find an instance of this function being overridden, so your concern is
valid:
http://www.google.com/codesearch/p?hl=en&sa=N&cd=1&ct=rc#1sfdY0UJBy4/trunk/op...

The "host" functionality is not being overridden, however, it is just used as a
hook to add an additional header.

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode52
Line 52: #
On 2009/06/20 16:48:46, Martin v. Löwis wrote:
> You will need to add a history line here.
I can do that, but it would appear that this has not been done  for five years
even though the file has been regularly updated.  SVN  is probably a more
reliable change log than inline comments.

[effbot, 2009-06-21 21:26:05]

LGTM.

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode1466
Line 1466: return self._connection
Why remove the nice error message?

[krisvale, 2009-06-24 21:13:28]

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode1466
Line 1466: return self._connection
On 2009/06/21 21:26:06, effbot wrote:
> Why remove the nice error message?
Because this is now part of the standard lib that does support HTTPS?  The
module appears to have been written originally to support a wide variety of
python versions, but that isn't necessary now that it's part of the lib.
But that change is, of course, quite unnecessary and has nothing to do with the
rest of the stuff, so I'll re add the try/except for completeness.

[effbot, 2009-06-24 21:24:02]

http://codereview.appspot.com/73041/diff/6010/5015
File Lib/xmlrpclib.py (right):

http://codereview.appspot.com/73041/diff/6010/5015#newcode1466
Line 1466: return self._connection
> Because this is now part of the standard lib that does support HTTPS?

SSL socket support is provided by an optional third-party library, via a
separate module (_ssl).  You can still build Python without it.  See the httplib
and socket module documentation (and code) for details

[krisvale, 2009-06-24 21:24:44]

I've made two changes as per your comments:
1) send_host has been reinstated
2) I've re-added the HTTPS attribute check, for completeness.

I also fixed:
1) Transport needs to remember the "host" parameter in order to cache the
"connection"
2) changed the ServerProxy.__call__() to be aan attribute getter.  one can thus
close it with:  p('close')() and access the transport with p('transport').  The
latter can be  particularly useful, for example if one subclasses Transport to
add functionality, such as keepalive pings and other stuff.