[Twisted-Python] ldaptor's LDAPClient.send_multiResponse not dealing with chained deferreds, patch
This message is probably for Tv. I have a situation where the callback to a LDAPClient.send_multiResponse() call needs to do a bunch of work that I want to split up over several functions, using deferred chaining. What I want to do is something like this (this in conjunction with an LDAP proxy server I am writing using Twisted and ldaptor): d = self.client.send_multiResponse(request, got_response) then later: def got_response(response): # get a list of other things to do with the response d = do_stuff_with_response(response) d.addCallback(finish) return d def finish(response): # we're done return isinstance(response, ( pureldap.LDAPSearchResultDone, pureldap.LDAPBindResponse, )) However, this doesn't work, because the send_multiResponse() method of LDAPClient isn't expecting the handler to return a deferred, even though if I do return a deferred the reactor does handle it properly, chaining the deferreds and their callbacks. The problem is that in ldapclient.py, line 171, we have: # Return true to mark request as fully handled if handler(msg.value, *args, **kwargs): del self.onwire[msg.id] When handler() returns a deferred object, after the reactor processes the chain the value is a Deferred object, not True or False, even though the value of the deferred object may be True or False. Hence the del self.onwire[msg.id] always executes, which when dealing with search result entries is a problem as they all share the same id. I made a quick hack to fix this: ----------------------------------------------------------- --- /tmp/ldapclient.py 2008-05-03 18:53:26.000000000 -0600 +++ ldaptor/protocols/ldap/ldapclient.py 2008-05-03 18:58:07.000000000 -0600 @@ -168,7 +168,14 @@ assert args is not None assert kwargs is not None # Return true to mark request as fully handled - if handler(msg.value, *args, **kwargs): + result = handler(msg.value, *args, **kwargs) + + try: + result = result.result + except AttributeError: + pass + + if result: del self.onwire[msg.id] ##Bind ---------------------------------------------------------- Is this an acceptable way to do this?
On Sat, 03 May 2008 19:03:29 -0600, Michael Torrie
This message is probably for Tv.
I have a situation where the callback to a LDAPClient.send_multiResponse() call needs to do a bunch of work that I want to split up over several functions, using deferred chaining. What I want to do is something like this (this in conjunction with an LDAP proxy server I am writing using Twisted and ldaptor):
d = self.client.send_multiResponse(request, got_response)
then later:
def got_response(response):
# get a list of other things to do with the response d = do_stuff_with_response(response) d.addCallback(finish)
return d
def finish(response): # we're done
return isinstance(response, ( pureldap.LDAPSearchResultDone, pureldap.LDAPBindResponse, ))
However, this doesn't work, because the send_multiResponse() method of LDAPClient isn't expecting the handler to return a deferred, even though if I do return a deferred the reactor does handle it properly, chaining the deferreds and their callbacks.
You don't need to return the Deferred in order for the code and event sources associated with it to cause it to eventually fire. Returning it doesn't do anything more than make it available to the caller. It seems this is what you actually want though, so this is just a change of perspective not a contradiction of your conclusion.
The problem is that in ldapclient.py, line 171, we have:
# Return true to mark request as fully handled if handler(msg.value, *args, **kwargs): del self.onwire[msg.id]
When handler() returns a deferred object, after the reactor processes the chain the value is a Deferred object, not True or False, even though the value of the deferred object may be True or False. Hence the del self.onwire[msg.id] always executes, which when dealing with search result entries is a problem as they all share the same id. I made a quick hack to fix this:
[snip] + result = handler(msg.value, *args, **kwargs) + + try: + result = result.result + except AttributeError: + pass + + if result: del self.onwire[msg.id]
##Bind ----------------------------------------------------------
Is this an acceptable way to do this?
Not really. You need to take the code that depends on the result of the Deferred and put it into a callback which gets attached to that Deferred. Something more like this: def cbHandler(result): if result: del self.onwire[msg.id] result = handler(msg.value, *args, **kwargs) if isinstance(result, Deferred): result.addCallback(cbHandler) else: cbHandler(result) Or, written using the helper function twisted.internet.defer.maybeDeferred, def cbHandler(result): if result: del self.onwire[msg.id] result = maybeDeferred(handler, msg.value, *args, **kwargs) result.addCallback(cbHandler) This correctly handles both Deferred and non-Deferred results. I don't have any idea if this change to ldaptor is correct with respect to LDAP semantics/requirements or the particular implementation details of the code in question here, though. Jean-Paul
Jean-Paul Calderone wrote:
However, this doesn't work, because the send_multiResponse() method of LDAPClient isn't expecting the handler to return a deferred, even though if I do return a deferred the reactor does handle it properly, chaining the deferreds and their callbacks.
You don't need to return the Deferred in order for the code and event sources associated with it to cause it to eventually fire. Returning it doesn't do anything more than make it available to the caller. It seems this is what you actually want though, so this is just a change of perspective not a contradiction of your conclusion.
This stuff is getting kind of confusing for me, I must confess. Especially because it's hard to explain exactly what I'm doing without posting a very large code example (with lots of supporting modules). I may work on a minimal example, based on the proxy.py that ldaptor ships with to demonstrate this. So yes, the handler is just a callback attached to a deferred that has already fired -- the deferred itself is irrelevant. This deferred was originally created by the LDAPClient.send_multiResponse() method, and it fires when a response to the request comes in (there can be more than one response, if the request was a search). This function is a bit confusing because it creates a deferred object that it returns to me, but also takes a handler function as an argument. I've tried to add my additional processing routines to this particular deferred as a callback, but that does not work. I think if I could get this to work, that might be the better way to go. In the meantime, in the handler I pass to send_multiResponse(), I need to generate a new deferred chain that has parts of it deferred to other threads, etc. Returning this new deferred from my handler seems ideal, as the reactor just does the right thing.
Or, written using the helper function twisted.internet.defer.maybeDeferred,
def cbHandler(result): if result: del self.onwire[msg.id] result = maybeDeferred(handler, msg.value, *args, **kwargs) result.addCallback(cbHandler)
This correctly handles both Deferred and non-Deferred results. I don't have any idea if this change to ldaptor is correct with respect to LDAP semantics/requirements or the particular implementation details of the code in question here, though.
Yes, this would seem the correct thing to do! I'll have to work out a patch that does this and see where we're at.
participants (2)
-
Jean-Paul Calderone -
Michael Torrie