Hi Martin,

Martin Geisler wrote:
All the tests in the buildbot are currently broken:


It is because of this change of yours:

@@ -164,6 +166,18 @@
         # the Runtime, since we want everybody to wait until all
         # runtimes are ready.
+        self.real_runtimes.append(runtime)
+        self.i = 0
+        # This loop call should ensure the queues of the parties are
+        # processed in a more or less fair manner. This is necessary
+        # because we have only one reactor for all parties here.
+        def loop_call():
+            for runtime in self.real_runtimes[self.i:] + 
+                runtime.process_deferred_queue()
+                self.i = (self.i + 1) % len(self.real_runtimes)
+        reactor.setLoopCall(loop_call)

I think the problem is that after this change the unit tests need the reactor to be a ViffReactor, but it seems that the reactor is a SelectReactor. And this seems to be the case since your changeset "Make the VIFF reactor available to trial".

I'm very confused about what exactly

  for x in xs[i:] + xs[:i]:
    i = (i + 1) % len(xs)

is supposed to do? After x has run through all xs (rotated i steps),
then i will have been incremented by len(xs). But you do it mod len(xs)
and so i comes out of the loop unchanged.

This solution is indeed not very elegant. The problem that has to be solved here is the following: In a normal run every player (i.e. every runtime) has its own reactor and this reactor runs process_deferred_queue() of one runtime. In the unit tests however, there is one reactor for n runtimes. Therefore the reactor has to call process_deferred_queue() of every runtime. Now, if we just would iterate as usual over the runtimes this would lead to very unbalanced and therefore slower execution, because the loop call is called recursively. So this construct is to ensure that every time loop_call() is called, another runtime has first priority.

The xs[i:] + xs[:i] construct is at best uncommon and at worst wasted
work. Instead of rotating the list, it should be enough to start at an
offset (i) and work your way through the list with proper wrapping at
the end.

You're right.

Also, the fact that you have both self.runtimes and self.real_runtimes
smells funny :-) It's as if you're circumventing the natural scopes of
the Deferreds by storing a reference to the runtimes "on the side".

The list of Deferreds in self.runtimes ought to be enough -- can you not
add callbacks to that list if you need something to happen when the
runtimes are ready?

Yes, it just was too lazy.

Best reagards,
viff-devel mailing list (http://viff.dk/)

Reply via email to