Opened 13 years ago

Closed 11 years ago

Last modified 9 years ago

#1895 enhancement closed wontfix (wontfix)

Add Native twisted.web2.xmlrpc Proxy Support

Reported by: oubiwann Owned by: dialtone
Priority: highest Milestone: Web2-Gold-Master
Component: web2 Keywords:
Cc: itamarst, oubiwann, David Reid, foom, Jean-Paul Calderone Branch:
Author:

Description

Currently, there is no twisted.web2.xmlrpc Proxy. The unit tests for t.w2.xmlrpc actually use t.w.xmlrpc.Proxy. Adding Proxy to t.w2.xmlrpc will also include adding QueryProtocol and QueryFactory.

Attachments (2)

xmlrpc_proxy-1895.xmlrpc.patch (5.9 KB) - added by oubiwann 13 years ago.
twisted.web2.xmlrpc with Proxy support
xmlrpc_proxy-1895.test_xmlrpc.patch (4.2 KB) - added by oubiwann 13 years ago.
updated tests for twisted.web2.xmlrpc with Proxy support

Download all attachments as: .zip

Change History (25)

comment:1 Changed 13 years ago by oubiwann

Status: newassigned

I will start working on this using dreid's t.w2.client.http code. I expect this will be somewhat of a moving target, but I have no problem with that, nor any extra work as a result... and I prefer keeping this alive, even if it bleeds for a while.

comment:2 Changed 13 years ago by oubiwann

A reference note: the completion of #1549 depends on this ticket.

comment:3 Changed 13 years ago by oubiwann

Summary: Add twisted.web2.xmlrpc Native Proxy SupportAdd Native twisted.web2.xmlrpc Proxy Support

I have a first draft of proxy code. In fact, the classes xmlrpc.Proxy and xmlrpc.QueryFactory have remained the same in web2 as they are in web. The only change I made was to rewrite xmlrpc.QueryProtocol.

I have added tests for the authentication as well as for the proxy, and all tests are passing.

I will upload web2.xmlrpc and test file patches from my branch for review.

Changed 13 years ago by oubiwann

twisted.web2.xmlrpc with Proxy support

Changed 13 years ago by oubiwann

updated tests for twisted.web2.xmlrpc with Proxy support

comment:4 Changed 13 years ago by oubiwann

Strange. All the tests passed, but when I try to run the example client (from the original XML-RPC howto) against advogato.org, it fails:

error [Failure instance: Traceback (failure with no frames): twisted.internet.error.ConnectionDone: Connection was closed cleanly.
]

Running with t.w.xmlrpc.Proxy works just fine, though.

I tried stepping through it with pdb, but nothing obvious jumped out at me. Here's where the error occurs:

> /Users/oubiwann/lab/DivmodAndTwisted/Twisted/xmlrpc_proxy-1895/twisted/internet/posixbase.py(228)mainLoop()
-> t = self.running and t2
(Pdb) next
> /Users/oubiwann/lab/DivmodAndTwisted/Twisted/xmlrpc_proxy-1895/twisted/internet/posixbase.py(229)mainLoop()
-> self.doIteration(t)
(Pdb) next
error [Failure instance: Traceback (failure with no frames): twisted.internet.error.ConnectionDone: Connection was closed cleanly.
]
> /Users/oubiwann/lab/DivmodAndTwisted/Twisted/xmlrpc_proxy-1895/twisted/internet/posixbase.py(223)mainLoop()
-> while self.running:

I'll ask David to take a look and see if I've done anything stupid.

comment:5 Changed 13 years ago by oubiwann

Well, this morning (I guess that's afternoon) with a clearer head, I did the common sense thing: a GET via HTTP. It seems that the followg works just fine on advogato.org:

GET / HTTP/1.0

whereas this generates a 400:

GET / HTTP/1.1

Are there any sort ot checks/backoffs in twisted.web2 for reconnecting to servers that don't support HTTP 1.1?

comment:6 Changed 13 years ago by oubiwann

Crap, I forget to add the Host line in the HTTP 1.1 request. Advogato handles HTTP 1.1 just fine. Please disregard that last comment.

comment:7 Changed 13 years ago by oubiwann

Cc: David Reid added

dreid and I are planning on working on this next week sometime. I've put the code I added in a pastebin for review:

http://pastebin.adytum.us/73

comment:8 Changed 13 years ago by oubiwann

Milestone: Web2-Gold-Master

comment:9 Changed 12 years ago by oubiwann

The unit test for this needs to be rewritten to not use the network (see #1929). In addition, there's only one Proxy-related unit test; there probably also should be one (or some) for QueryProtocol and QueryFactory.

comment:10 Changed 12 years ago by dialtone

Cc: foom added
Keywords: review added
Priority: normalhighest

Branch xmlrpc_proxy-1895 is ready for review with more tests added and network usage removed.

comment:11 Changed 12 years ago by dialtone

Owner: changed from oubiwann to dialtone
Status: assignednew

comment:12 Changed 12 years ago by dialtone

Owner: changed from dialtone to jknight

comment:13 Changed 12 years ago by jknight

Oops, didn't see this one waiting for me when I got back from vaca. I'll check it out soon.

comment:14 Changed 12 years ago by jknight

This looks wrong; HTTPClientProtocol isn't meant to be subclassed to be used.

comment:15 Changed 12 years ago by jknight

Keywords: xmlrpc web2 http client review removed
Owner: changed from jknight to dialtone

I wonder why the client is called "Proxy". Is there some deep reason it shouldn't be called a Client? Cause it really seems like a client to me.

ValueError doesn't seem a particularly appropriate exception for non-200 HTTP error statuses. I don't see http error responses being tested anywhere.

stream.read() doesn't read the entire stream, so _cbGotResponse won't work on long responses over a real TCP connection.

There don't seem to be any end-to-end workingness tests, only unit tests. It seems like it'd be valuable to actually verify a real connection to a real server. (doing so might've caught the previous error.) Also that'd allow a test of callRemote, which is currently completely untested.

I know some of those are inherited from web1's xmlrpc code, but it doesn't mean they can't be fixed this time around.

comment:16 Changed 12 years ago by dialtone

Keywords: review added

I renamed the Proxy object to Client.

ValueError exception is now changed into BadResponseCoreError(code, message) deriving from Exception and I've added a test with a real connection that tests this error.

There's now a real end to end test (I think it's enough) testing what problems might be if a "hello world"*1024 long string is returned by an xmlrpc method and it works it seems.

branch back in review

comment:17 Changed 12 years ago by dialtone

Owner: changed from dialtone to jknight

comment:18 Changed 12 years ago by David Reid

Why doesn't a non-200 responsecode just result in an HTTPError?

comment:19 Changed 12 years ago by David Reid

Owner: changed from jknight to David Reid
Status: newassigned

comment:20 Changed 12 years ago by David Reid

Keywords: review removed
Owner: changed from David Reid to dialtone
Status: assignednew
  • I am still inclined to believe that BadResponseCodeError should just be an HTTPError.
  • I also agree with James that this isn't how HTTPClientProtocol was meant to be used. You should create a protocol, in this case probably just
  • Also you're an HTTP/1.1 client now, and creating a new connection for every method call without specifying Connection: close. Which means you're leaking persistent connections. I'd like to say we should create one connection and reuse it for multiple method calls, then recreate it on demand when it times out ... but that brings me to my next point.
  • Does the XML-RPC "specification" even let you have an HTTP/1.1 client? It seems like a lot of servers in the wild probably aren't 1.1 servers.

comment:21 Changed 12 years ago by itamarst

I would expect most XML-RPC servers to be 1.1 at this point; even if not, HTTP 1.1 clients should be able to interop with 1.0 servers.

Ideally the client class (as with most uses of HTTP client) should manage a pool of connections; 1 may be a good default size. However, that's generic HTTP client infrastructure, not XML-RPC specific, so it should probably be done elsewhere.

comment:22 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

Suggesting close as wontfix. Effort should go towards improve twisted.web.client and twisted.web.xmlrpc.

comment:23 Changed 11 years ago by Jean-Paul Calderone

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.