Opened 2 years ago

Last modified 11 months ago

#5732 enhancement new

Support XML-RPC multicalls

Reported by: vinodh Owned by: braudel
Priority: normal Milestone:
Component: web Keywords:
Cc: bmi@…, thijs, richard@…, patrick@… Branch: branches/xmlrpc-multicall-5732-3
(diff, github, buildbot, log)
Author: braudel, rwall Launchpad Bug:

Description (last modified by exarkun)

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.

Attachments (5)

multicall.patch (7.7 KB) - added by braudel 2 years ago.
patch to add support for multicall
multicall2.patch (16.7 KB) - added by braudel 2 years ago.
updated patch with individual deferred per queued rpc
multicall2.2.patch (18.7 KB) - added by braudel 2 years ago.
corrected as per Glyph's feedback
multicall2.diff (19.5 KB) - added by braudel 2 years ago.
Corrected errors in documentation, docstrings, etc…
multicall3.diff (17.0 KB) - added by braudel 13 months ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 2 years ago by exarkun

  • Component changed from core to web
  • Description modified (diff)
  • Summary changed from server.register_multicall_functions() Inclusion! to Support XML-RPC multicalls

Changed 2 years ago by braudel

patch to add support for multicall

comment:2 Changed 2 years ago by braudel

  • Cc bmi@… added

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

comment:3 Changed 2 years ago by exarkun

  • Owner set to braudel

Thanks. Please read ReviewProcess.

Changed 2 years ago by braudel

updated patch with individual deferred per queued rpc

comment:4 Changed 2 years ago by braudel

  • Author set to braudel
  • Keywords review added
  • Owner braudel deleted

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

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

comment:5 Changed 2 years ago by glyph

  • Keywords review removed
  • Owner set to braudel

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
  1. Docstrings are typically written with triple-double-quotes, i.e. """.
  1. 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.
  1. There should be 2 blank lines between methods, and 3 between classes.
  1. 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.
  1. 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

Changed 2 years ago by braudel

corrected as per Glyph's feedback

comment:6 Changed 2 years ago by braudel

  • Keywords review added
  • Owner changed from braudel to glyph

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

comment:7 Changed 2 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner changed from glyph to braudel

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
  1. 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
      

Changed 2 years ago by braudel

Corrected errors in documentation, docstrings, etc...

comment:8 Changed 2 years ago by braudel

  • Keywords review added
  • Owner changed from braudel to thijs

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

comment:9 Changed 22 months ago by thijs

  • Owner thijs deleted

comment:10 Changed 20 months ago by rwall

  • Cc richard@… added
  • Owner set to rwall
  • Status changed from new to assigned

comment:11 Changed 20 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to braudel
  • Status changed from assigned to new

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.

comment:12 Changed 17 months ago by rwall

  • Author changed from braudel to braudel, rwall
  • Branch set to branches/xmlrpc-multicall-5732

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

comment:13 Changed 17 months ago by rwall

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

comment:14 Changed 17 months ago by 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.

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:

Thanks again.

-RichardW.

comment:15 Changed 17 months ago by rwall

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

comment:16 Changed 14 months ago by braudel

Hi rwall,

My apologies for taking so long to respond, but things have been hectic on my side. I have finally started to work on your suggestions. I hope to have a patch soon.

Changed 13 months ago by braudel

comment:17 follow-up: Changed 13 months ago by braudel

  • Keywords review added
  • Owner changed from braudel to rwall

Dear Rwall,

Please find above the path multicall3.diff that attempts to satisfy your previous concerns. Some remarks to note regarding your questions in 11):

In xmlrpc.py:
1) xmlrpc_multicall docstring

  • Removed example as suggested.
  • No point in an example showing how to load introspection since this is available elsewhere

4.2) Should "run" be a private method rather than a closure? Not sure what advantage will this bring. The run method is only needed within that specific context

6.1, 6.2, 6.3) added 'run' method in addition to the xmlrpclib style of calling

6.4) I find hard to see why would you call n-methods in the server side (you will have to wait until all them finish before the rpc results come back) to then jump out at the first one that processes its callbacks and discard the rest? However I may be missing some use cases, so I am adding the deferredList args as per your suggestion.

6.6) I am affraid that I disagree on this one. Why take the power of the deferreds out of the way? Let the coder to decide whether to get raw results (just don't add callbacks/errbacks to any deferred returned by .callRemote) or to play with individual callbacks, etc as per any typical twisted stuff.

In test_xmlrpc.py:
3) Splitting test_multicall_errorback into two feels like a non-neccesary duplication. This test checks that an error raised during the execution of a rpc in the list is "handled" by the errorback associated to its deferred and does not propagates up if handled by the errorback. I decided to try two different "mistakes" as a way of testing at the same time that more than 1 failure in the rpc list are handled as expected (of course I could have used the same type of mistake twice). I believe that the testing of the proper raising of "not found" errors, etc is not matter of the multicall code itself.

4) When an RPC method is decorated in the resource with @withRequest, the xmlrpc resource calls the rpc method passing the request object as its first argument. The xmlrpc_multicall "emulates" the xmlrpc resource's dispatcher, hence this test to ensure that the code is also honouring the @withRequest decorator when needed.

Regards,

Brau

comment:18 Changed 13 months ago by rwall

(In [39317]) Apply multicall3.diff from braudel. refs #5732

comment:19 Changed 13 months ago by rwall

  • Branch changed from branches/xmlrpc-multicall-5732 to branches/xmlrpc-multicall-5732-2

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

comment:20 Changed 13 months ago by rwall

(In [39319]) merge forward. refs #5732.

comment:21 follow-up: Changed 13 months ago by rwall

  • Owner rwall deleted

Hi braudel,

I think I agree with your points and responses above. I don't know this code too well so there may have been misunderstandings in my previous review.

Anyway, thanks for the updated patch. I applied it, merged forward and started a new build.

I haven't got time to do a proper review this time, but I do notice a few cosmetic things that need fixing:

  1. http://buildbot.twistedmatrix.com/builders/pyflakes/builds/763/steps/pyflakes/logs/new%20pyflakes%20errors
  2. http://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1052/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  3. Various methods and variable names should be camelCase as described in the coding standard: https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html
  4. Use single underscore prefix for private attributes.
  5. See #6301 for the Twisted policy on writing test method docstrings.
  6. Some missing test coverage of your multicall code. Add some extra tests: http://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r39319/twisted_web_xmlrpc.html

Please submit another patch - this time against source:branches/xmlrpc-multicall-5732-2

Thanks again for moving things forward and sorry for not doing a proper review this time.

I'll leave the ticket in review so that someone else can take a proper look at the code.

comment:22 in reply to: ↑ 17 Changed 13 months ago by psmears

  • Cc patrick@… added

Hi Brau,

Thanks for your work on this - I'm currently working on a project that would have
stalled if your diff hadn't been here!

I think there may be a small bug in the diff as it stands. I'm not 100% familiar
with the code, so I may be wrong, but I think that, while the diff handles
errors being returned for the individual calls, it doesn't cope with the
entire request failing (e.g. due to an HTTP error code coming back).

In the code:

self.server.callRemote(

'system.multicall', marshalled_list

).addCallback(cbProcessResults, deferreds)

there is no Errback defined - and no way for the caller to add one (since this
deferred is not returned) - so if there is an HTTP error, it will go unhandled.

I think what's needed is to add another function, similar to processResults, that
propagates the failure to all of the individual Deferreds, that can then be
registered as an Errback on this Deferred.

I hope that's at least sort of clear; let me know if not - and thanks again for
your work in putting this together in the first place! Patrick

comment:23 in reply to: ↑ 21 Changed 13 months ago by thijs

  • Owner set to braudel

Replying to rwall:

Hi braudel,

Please submit another patch - this time against source:branches/xmlrpc-multicall-5732-2

Thanks again for moving things forward and sorry for not doing a proper review this time.

I'll leave the ticket in review so that someone else can take a proper look at the code.

comment:24 Changed 12 months ago by rwall

Closed duplicate ticket #3823

comment:25 Changed 12 months ago by rwall

  • Owner braudel deleted

Hi Braudel,

Haven't heard back from you for a while.

There's been some further feedback from another user since my (partial review).

So please address the points in my review (comment:21) and answer comment:22 if you can.

This branch is nearing the top of the review queue, so it would be nice to have the cosmetic stuff fixed by the time someone with xml-rpc expertise comes to do a final technical review.

And log into #twisted-web if you want to discuss anything.

Look forward to hearing from you.

-RichardW.

comment:26 Changed 12 months ago by rwall

  • Branch changed from branches/xmlrpc-multicall-5732-2 to branches/xmlrpc-multicall-5732-3

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

comment:27 Changed 12 months ago by rwall

(In [39913]) merge forward, resolve conflicts. refs #5732.

comment:29 Changed 11 months ago by tom.prince

  • Keywords review removed
  • Owner set to braudel

Thanks for your persistence in working on this.

Some comments on the api.

  1. Once a deferred is given to user code (for example, returned from a method), the code that created that code shouldn't add further callbacks (which is what DeferredList does). I think MultiCall.run should just return a defered that fires with None, and let the values be exposed by the invividual deferreds.
  1. This would address psmears issue about the lack of errback on the callRemote('system.multicall', ...).
  2. In the case of an error from 'system.multicall', all the individual deferreds should be errbacked.
  1. I think the __call__ and __getattr__ are too magic, and should be removed. The convention in twisted is for remote method calls to be explicit.
  1. There typically isn't any reason to use __ prefixed names over _ prefixed names. They make the names harder to type when debugging, but don't otherwise prevent them from being referenced.

Code:

  1. The api documentation should not typically mention that a DeferredList is being returned, particularly if callbacks have been added that change the shape of the result. A DeferredList has the same interface as a Deferred, and the fact that one was used is an implementation detail which shouldn't be exposed.
  1. (minor) docstrings should begin with a captital and be complete sentence or sentences.
  1. It would be good to have a reference to the RFC. I'm not familiar with xmlrpc, but the following two links appear to be candidates.
  1. I wonder if the logic for calling xmlrpc_ methods could usefully be factored between xmlrpc_multicall and render_POST. Particularly the withRequest handling.

I haven't looked at the tests in detail, but

  1. (minor) The test method names should follow the naming policy: In particular they should be camelCased with a test_ prefix.
  2. The docstrings for the test methods could be improved (see #6301) for some discussion. In particular, they should avoid saying something is done "correctly", and instead specify what specific behavior is expected.

Please resubmit for review after responding to or addressing the above points.

Note: See TracTickets for help on using tickets.