Opened 11 years ago

Last modified 11 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, moshez, icepick Branch:
Author: Launchpad Bug:

Description


Attachments (2)

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

Download all attachments as: .zip

Change History (16)

Changed 11 years ago by icepick

comment:1 Changed 11 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 11 years ago by moshez

This is weird. When deferreds are passed a deferred as a value, it should 
automatically undefer itself...so 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 11 years ago by icepick

Sure.  My chord unit test makes use of this:  

http://cryptomonkey.net/dht/chord.py
http://cryptomonkey.net/dht/test_chord.py

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

comment:4 Changed 11 years ago by moshez

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 11 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 11 years ago by moshez

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 11 years ago by icepick

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

Changed 11 years ago by icepick

comment:8 Changed 11 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 11 years ago by icepick

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

comment:10 Changed 11 years ago by moshez

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.

Thanks

Index: twisted/spread/util.py
===================================================================
RCS file: /cvs/Twisted/twisted/spread/util.py,v
retrieving revision 1.10
diff -u -r1.10 util.py
--- twisted/spread/util.py      4 May 2003 12:03:34 -0000       1.10
+++ twisted/spread/util.py      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:
             return defer.fail(
Index: twisted/test/test_spread.py
===================================================================
RCS file: /cvs/Twisted/twisted/test/test_spread.py,v
retrieving revision 1.3
diff -u -r1.3 test_spread.py
--- twisted/test/test_spread.py 11 Feb 2003 23:45:52 -0000      1.3
+++ twisted/test/test_spread.py 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):
         pass
+    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
         lf.callRemote("dontForwardMe")
         assert not f.unforwarded
+        rr = lf.callRemote("forwardDeferred")
+        l = []
+        rr.addCallback(l.append)
+        self.assertEqual(l[0], 1)

comment:11 Changed 11 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 11 years ago by moshez

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

Fixed.

comment:13 Changed 11 years ago by icepick

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

comment:14 Changed 3 years ago by <automation>

  • Owner moshez deleted
Note: See TracTickets for help on using tickets.