Opened 9 years ago

Closed 4 years ago

#3823 enhancement closed duplicate (duplicate)

Add system.multicall support for the XML-RPC server

Reported by: ramen Owned by: ramen
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch:
Author:

Description

The Twisted XML-RPC server does not come with system.multicall, a de facto standard method which allows a batch of methods to be called in a single request.

I wrote an implementation which handles deferred results and ensures that exceptions in one method call do not prevent the other methods from executing.

Attachments (1)

xmlrpc-multicall.patch (3.9 KB) - added by ramen 9 years ago.
system.multicall implementation with unit test

Download all attachments as: .zip

Change History (6)

Changed 9 years ago by ramen

Attachment: xmlrpc-multicall.patch added

system.multicall implementation with unit test

comment:1 Changed 9 years ago by ramen

Keywords: review added
Owner: changed from Glyph to ramen

I originally posted this to the twisted-web list, and Glyph responded: http://twistedmatrix.com/pipermail/twisted-web/2009-May/004170.html

Documentation for system.multicall used to be available here: http://www.xmlrpc.com/discuss/msgReader$1208

Unfortunately, this is now a 404. You can still view the spec through the Wayback Machine: http://web.archive.org/web/20060824100531/http://www.xmlrpc.com/discuss/msgReader$1208

I have added the Wayback Machine link to the docstring for the time being, along with a description of the calling conventions for system.multicall. I did not use the @param keyword because the docstring is used as a system.methodHelp result and it doesn't seem like any of the other RPC methods use @param (for this reason, I'm guessing).

I wrote a unit test and ran into some snags with error handling. I call log.err() from the system.multicall handler when there is an exception, and this seems to make the test fail with ERRORs no matter what I do (please see the comments at the top of the unit test). If an exception is thrown, it should be logged, but the call to system.multicall should be a success with faults embedded in it. I'm not sure how to communicate to the test framework that this is a success despite the error messages.

Regarding the use of deferredGenerator and its effects on parallelization: My first attempt at writing system.multicall resulted in a method that could not handle RPC methods returning deferreds at all because xmlrpclib complained that they could not be serialized. As xmlrpc.py is written, XML-RPC serialization is done on the entire result, which can be a deferred or not, but if there are deferreds nested inside of the result, they are not explicitly forced. My solution was to use deferredGenerator to ensure that all of these deferreds are forced before the result is generated, but as Glyph pointed out, this forces the methods to be called one at a time, and there is an obvious potential to parallelize the calls which we are missing out on.

I wonder if it would be possible to make all the deferred calls first, in one loop, and collect the deferred results. Then, in a second loop, the defer.waitForDeferred / yield calls could be done to collect the results. I'm not sure how deferredGenerator works, so I don't know if this would allow the calls to be parallelized or not. Maybe someone can help me out here.

Thanks for your time, and let me know if I can clarify anything further!

Dave

comment:2 Changed 9 years ago by ramen

Owner: ramen deleted

comment:3 Changed 9 years ago by ramen

I forgot to mention that Python's standard xmlrpclib module contains a multicall client, which can also be used to test (though I'm not sure how well it mixes with Twisted's asynchronous XML-RPC client).

http://docs.python.org/library/xmlrpclib.html#multicall-objects

comment:4 Changed 9 years ago by Glyph

Keywords: review removed
Owner: set to ramen

Hi Dave,

Thanks again for your contribution. I have some more feedback for you:

  1. Please generate patches against the root of the repository; as described on the page about twisted development, that's the standard location so that other developers don't need to read your patch to figure out where to apply it. In other words, if you do "svn co svn://svn.twistedmatrix.com/svn/Twisted/trunk MyCopyOfTwisted", just do "cd MyCopyOfTwisted; svn diff > my.patch" to generate your patch.
  2. Regarding your question about errors: the Twisted test framework provides a mechanism specifically for doing this. You can get the errors that were logged and assert things about them. The idea is that you don't silence errors, since error logging is important; you test that the appropriate errors were logged. Here's a brief example:
    from twisted.python import log
    
    def errorLoggingAPI():
        try:
            1 / 0
        except:
            log.err()
    
    from twisted.trial.unittest import TestCase
    
    class ErrorTester(TestCase):
        def test_errorLogging(self):
            errorLoggingAPI()
            failures = self.flushLoggedErrors(ZeroDivisionError)
            self.assertEquals(len(failures), 1)
    
  3. Given that multicall has more than one behavior (at the very least, it's got success-path and error-path) there should be more than one test. Each test should have a docstring, explaining what it's verifying. The surrounding test code in this module is out date, so it doesn't set a terribly good example; I encourage you to read the coding standard to see what requirements for new code are like.
  4. Regarding deferredGenerator, that feedback from my email stands. I don't think you want to use deferredGenerator at all; more likely, you want to use gatherResults. It works like this:
    from twisted.internet.defer import succeed, gatherResults
    d = gatherResults([succeed(1), succeed(2), succeed(3)])
    def showme(sequence):
        print 'Results:', sequence
    d.addCallback(showme)
    
  5. In your docstring, you refer to a dictionary as a 'struct' - this might be a little confusing; I'd just call it a dict or a dictionary.

I hope you'll be back with a new patch soon! :)

comment:5 Changed 4 years ago by Richard Wall

Component: coreweb
Resolution: duplicate
Status: newclosed

See duplicate ticket #5732.

That ticket has a more recent implementation in a branch.

Note: See TracTickets for help on using tickets.