Opened 13 years ago

Last modified 13 years ago

#286 defect closed fixed (fixed)

Fix for t.spread.util.LocalAsyncFowarder

Reported by: icepick Owned by:
Priority: high Milestone:
Component: Keywords:
Cc: itamarst, Moshe Zadka, icepick Branch:


Attachments (2)

t.spread.util.LocalAsyncForwarder.patch (1.0 KB) - added by icepick 13 years ago.
test_spread.patch (1.0 KB) - added by icepick 13 years ago.

Download all attachments as: .zip

Change History (16)

Changed 13 years ago by icepick

comment:1 Changed 13 years ago by icepick

LocalAsyncForwarder was returning Deferred's to my when in a
real network it wouldn't, so I fixed it.

comment:2 Changed 13 years ago by Moshe Zadka

This is weird. When deferreds are passed a deferred as a value, it should 
automatically undefer basically, _stripDeferred should be a no-op. 
Can you find a test case which demonstrates this problem? Because I think the 
solution should be elsewhere.

comment:3 Changed 13 years ago by icepick

Sure.  My chord unit test makes use of this:

If you run it thru 'trial' without this patch you get errors of callback getting
Deferreds with results as the arg.

comment:4 Changed 13 years ago by Moshe Zadka

Can you attempt to minimize the error? I can't run these, because I lack a significant number of dependencies, and I'm pretty certain this can't be related to, say, pycrypto :)

comment:5 Changed 13 years ago by itamarst

I think the issue is the code does myDeferred.callback(o) where o might be a
Deferred as well. If o is a Deferred this is incorrect behaviour.

comment:6 Changed 13 years ago by Moshe Zadka

He doesn't seem to call .callback himself, so there would appear to be an error in Twisted somewhere.
Hence, my request for a bug report, rather than a patch.
icepick, in general, we need a bug report before we can evaluate patches. Ideally, a failing test case which we can run is best -- this allows us to add it to the Twisted test suite to ensure the bug doesn't happen again [or to see that you are using an API incorrectly and add a warning to the documentation].

comment:7 Changed 13 years ago by icepick

I'll work on a simple test case, probably this weekend.

Changed 13 years ago by icepick

Attachment: test_spread.patch added

comment:8 Changed 13 years ago by icepick

This was bugging me, and the test is simple.  Here is a patch for the unit test
that shows my problem.

comment:9 Changed 13 years ago by icepick

The last patch is incorrect.  Working on a new one.

comment:10 Changed 13 years ago by Moshe Zadka

The following patch would appear to solve the problem correctly.
Assigning to Itamar for review, feel free to assign back to me for
applying the patch, or apply it yourself.


Index: twisted/spread/
RCS file: /cvs/Twisted/twisted/spread/,v
retrieving revision 1.10
diff -u -r1.10
--- twisted/spread/      4 May 2003 12:03:34 -0000       1.10
+++ twisted/spread/      2 Oct 2003 19:01:31 -0000
@@ -77,7 +77,7 @@

     def callRemote(self, method, *args, **kw):
         if hasattr(self.interfaceClass, method):
-            result = defer.execute(self._callMethod, method, *args, **kw)
+            result = defer.maybeDeferred(self._callMethod, method, *args, **kw)
             return result
         elif self.failWhenNotImplemented:
Index: twisted/test/
RCS file: /cvs/Twisted/twisted/test/,v
retrieving revision 1.3
diff -u -r1.3
--- twisted/test/ 11 Feb 2003 23:45:52 -0000      1.3
+++ twisted/test/ 2 Oct 2003 19:01:31 -0000
@@ -22,11 +22,14 @@
 from twisted.trial import unittest

 from twisted.spread.util import LocalAsyncForwarder
+from twisted.internet import defer
 from twisted.python.components import Interface

 class IForwarded:
     def forwardMe(self):
+    def forwardDeferred(self):
+        pass

 class Forwarded:

@@ -39,8 +42,9 @@

     def dontForwardMe(self):
         self.unforwarded = 1
+    def forwardDeferred(self):
+        return defer.succeed(1)

 class SpreadUtilTest(unittest.TestCase):
     def testLocalAsyncForwarder(self):
@@ -50,3 +54,7 @@
         assert f.forwarded
         assert not f.unforwarded
+        rr = lf.callRemote("forwardDeferred")
+        l = []
+        rr.addCallback(l.append)
+        self.assertEqual(l[0], 1)

comment:11 Changed 13 years ago by itamarst

I can't get the patch to apply the diff, dunno why. Feel free to do it yourself
and commit.

comment:12 Changed 13 years ago by Moshe Zadka

Checking in twisted/spread/;
/cvs/Twisted/twisted/spread/,v  <--
new revision: 1.11; previous revision: 1.10


comment:13 Changed 13 years ago by icepick

I applied the patch to my tree and it works.  Thanks.

comment:14 Changed 6 years ago by <automation>

Owner: Moshe Zadka deleted
Note: See TracTickets for help on using tickets.