Support XML-RPC multicalls

[vinodh]

There is a commonly supported XML-RPC extension for calling multiple procedures with a single HTTP request. This is commonly exposed as "system.multicall". It's not clear whether a standard exists to define how it operates, but lots of implementations exist.

[braudel]

patch to add support for multicall

[braudel]

Hi there,

Please find attached a patch to add support for multicall for your consideration. Feels free to ask for further modifications/corrections as needed...

Regards, Braudel

[exarkun]

Thanks. Please read ReviewProcess.

[braudel]

updated patch with individual deferred per queued rpc

[braudel]

Please ignore/delete first version (multicall.patch).

The second attachment multicall2.patch is a more complete implementation, supporting deferred handling for each queued call.

[glyph]

Hi again braudel!

Thanks for following up on your patch. This is a great attempt. I'm particularly pleased to see new narrative documentation to accompany. I've got some feedback for you, which I hope you'll have time to address in the patch.

1. All new code suites except for callbacks - test suites, test methods, private methods, new APIs - need to have docstrings on them. So, for example, XMLRPCTestMultiCall

2. Docstrings are typically written with triple-double-quotes, i.e. """.

3. You've left some trailing whitespace in your patches. Your editor will probably have a thing that will let you either view or automatically delete trailing spaces; have a look at that.

4. There should be 2 blank lines between methods, and 3 between classes.

5. Newer tests should not spin the reactor. This means that, wherever possible, you should not be returning Deferred objects from your tests; instead, you should be using utilities like twisted.test.proto_helpers.MemoryReactor. Your tests can then assert that your returned Deferred should have already been called after a particular event occurs (like, the repsonse being received from the server), rather than hoping it gets called and then asserting that some stuff happens if it doesn't.

6. The new narrative documentation forgets the class="python" on the second <pre>.

Thanks again for your submission, and I hope you'll stick with it and help us get this functionality into Twisted :).

-glyph

[braudel]

corrected as per Glyph's feedback

[braudel]

Hi glyph,

Many thanks for your feedback. I have uploaded an updated multicall2.patch that I hope will address your corrections.

I could not work out exactly how to use the twisted.test.proto_helpers.MemoryReactor in my tests as suggested. But I have re-written the tests to be networkless and removed the return of deferreds. I hope this will address correctly your point #5. However I welcome very much your opinion on my approach, since I am not sure if there may be a better way of achieving this.

Please feel free to request more correction/changes as needed.

Kind regards,

Braudel

[thijs]

Thanks braudel. I tested the examples against the trac 0.12  xmlrpc plugin and noticed the following:

1. In xmlrpc.py:

1. MultiCall is missing from __all__
2. MultiCall should have a @since: 12.3 epytext marker
3. The example in the docstring of xmlrpc_multicall contains a NameError. multiRPC = Multicall( proxy ) should be multiRPC = MultiCall(proxy)
4. The docstring for MultiCall contains incorrect epytext:

       @type server should be a twisted xmlrpc L{Proxy} object.
   

   should be:

       @type server: L{Proxy}
   

5. The examples in the docstrings are incorrectly formatted. Check the foo2bar example in the 'Docstrings' section of the coding standard on how to document examples.
6. Sentences should start with a capital and end with a period

2. In xmlrpc.xhtml:

1. References to code should be linked to the API page. So <code>MultiCall</code> becomes <code class="API">twisted.web.xmlrpc.MultiCall</code>
2. The example contains a SyntaxError:

           multiRPC.echo(x).addCallback(lamdba x: x*x)
                                           ^
       SyntaxError: invalid syntax
   

[braudel]

Corrected errors in documentation, docstrings, etc...

[braudel]

Hi thijs,

Many thanks for your corrections. Please find attached the latest version of the patch where I try to address all your suggestions (I hope I haven't missed any).

Kind regards,

Braudel

[rwall]

Code Review

Braudel - Your ticket was at the top of the review list so I took a look. I'm not an expert in XML-RPC so excuse me if I misunderstood anything.

I haven't got commit access, but IMHO the new MultiCall code looks extremely useful and should be almost ready to merge. Here are a few comments:

1. Add a topfile with a summary of the new features. See

wiki:ReviewProcess#Newsfiles

2. Glyph already mentioned it but there is still some trailing

whitespace in your last patch.

3. flake8 complains that there are some long lines (>79) in the xmlrpc

and test_xmlrpc modules. Rewrap all the new bits.

4. Update @since API version strings. 12.3 has already been released. 5. Various inconsistencies in the epydocs - use of C{} and L{} in

@param and @type (specific examples below)

6. Strictly speaking I don't think you need to document nested

functions / closures, and if you do, I don't know whether it will show up in the pydoctor api docs. Eg the new task.react feature (by exarkun) doesn't document the nested callback functions. source:tags/releases/twisted-12.3.0/twisted/internet/task.py#L790

twisted/web/xmlrpc.py

1. "xmlrpc_multicall" docstring:

1. The example code in this method seems unnecessary.
2. It demonstrates the MultiCall client rather than the server.
3. typo "On the server side, just load the instrospection"
4. An example showing how to "load the instrospection" would be more useful.
5. Use L instead of C "http C{request}"

2. "def callError(error):"

1. Wrong @param name and use L instead of C " @param result: C{failure}"
2. Why not just pass the Failure instance to log.err instead of log.err(error.value) See  http://twistedmatrix.com/documents/12.2.0/core/howto/logging.html#auto0

3. "def prepareCallResponse(result):"

1. "@type result: Any python type." is that true? Aren't there restrictions on the return types?

4. "def run(procedurePath, params):"

1. "returns a C{deferred}" should it be an L?
2. Personally, I think run should be a private method rather than a closure.

5. "class MultiCall(xmlrpclib.MultiCall):"

1. epydoc lines have be at the end of the docstring...I think.

6. "def call":

1. I personally don't like the use of call. I find it confusing when objects turn out to be callable.
2. I understand that this provides consistency with the stdlib module, but...
3. I think it would be nice to add an alternative explicit method eg "execute" and call that from call
4. If you return a DeferredList, I think the user also needs to be able to choose the DeferredList parameters eg fireOnOneCallback etc see  http://twistedmatrix.com/documents/12.3.0/api/twisted.internet.defer.DeferredList.html#__init
5. eg what if you add a long running callback to an individual procedure - the multicall deferredList won't fire until all the individual callback chains have finished. But I guess you often just want to know that you're multicall has returned, so that you can issue another xmlrpc call.
6. Also, I think it seems confusing that the callbacks on individual procedure deferreds can affect the results of the multicall. Wouldn't it be better if the multicall deferredList always represented the raw results of the individual procedures?

7. "def getattr"

1. docstring should start on a new line.
2. this breaks various standard stuff like dir and help. Can you make it more specific eg

      In [15]: help(multiRPC)
      ...
      TypeError: argument of type '_DeferredMultiCallProcedure' is not iterable

      In [15]: dir(multiRPC)
      ---------------------------------------------------------------------------
      TypeError                                 Traceback (most recent call last)
      <ipython-input-15-a7c0799a770d> in <module>()
      ----> 1 dir(multiRPC)

      TypeError: __dir__() must return a list, not instance

twisted/web/test/test_xmlrpc.py

1. "self.assertTrue(resultsDeferred.called)" Adding this at the end of

every test seems repetitive and unnecessary. Consider removing it from the tests.

2. "def test_multicall"

1. Typo "test a suscessfull"
2. ...and the same in the next test method.

3. "def test_multicall_errorback"

1. I think this should be split into separate tests eg
2. test_procedure_not_found
3. test_wrong_arguments

4. "test_multicall_withRequest"

1. I don't understand this test, particularly m.withRequest(msg) Consider adding some comments to explain what's going on.

5. "test_multicall_with_xmlrpclib"

1. I think you should make PatchedXmlrpclibProxy to the module scope rather than a nested class
2. The test docstring should explain what exactly it means to be "compatible with xmlrpclib MultiCall client"

Ok, that's all for now.

-RichardW.

[rwall]

(In [38210]) Branching to 'xmlrpc-multicall-5732'

[rwall]

(In [38211]) apply multicall2.diff from braudel. refs #5732

[rwall]

Hi Braudel.

I've been given commit access now so I've applied your latest patch to a branch and started a new build.

• log:branches/xmlrpc-multicall-5732
•  https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/xmlrpc-multicall-5732

If you find time to address (or answer) the code review comments (above) and create an updated patch against the new branch it would be greatly appreciated.

Note: There are a few additional items that I missed in my code review which have been found by twistedchecker. These need addressing too:

•  https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/613/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

Thanks again.

-RichardW.

[rwall]

Also worth noting that there are one or two bits of multicall code without test coverage:

•  https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r38211/twisted_web_xmlrpc.html