On 12-09-04 04:40 PM, Barry Warsaw wrote:
>> +    def __call__(self, operation, *args, **kwargs):
>> +        """Use threads to avoid blocking on I/O."""
>> +        Thread(target=getattr(self, operation),
>> +               args=args, kwargs=kwargs).start()
>
> You probably want to make this a daemon thread, so that if it doesn't
> complete, it won't prevent exiting of the main process.

Ah, ok. I wasn't sure what daemon mode meant specifically. That does 
sound reasonable.

> Here's what I plan on committing:
>
>      def __call__(self, operation, *args, **kwargs):
>          """Use threads to avoid blocking on I/O."""
>          # Let AttributeErrors propagate up.
>          method = getattr(self, operation)

Do you really need to assign to this variable called 'method'? It's a 
pet peeve of mine to assign variables that only get used once. Python 
programmers are obsessed with doing this and I can't figure out why. I 
guess it helps readability but it does introduce a performance penalty, 
even if minor.

>          thread = Thread(target=method, args=args, kwargs=kwargs)
>          thread.daemon = True
>          thread.start()
>
> Although it might be better to squirrel the thread away in an instance
> variable, so that it can be directly joined, instead of having to loop through
> threading.enumerate() as in your test.

I don't like it being an instance variable, that keeps a reference to it 
around that keeps it alive after it exits. I like the idea of the thread 
being "inaccessible" in the background, just like when you are using 
some other asynch API (like GObject or even expat or whatever), you 
don't have any way of polling it until it calls your callback.

The other problem with it being an instance variable is that __call__ is 
going to get called a lot, thus is going to make a bunch of threads, so 
you can't just have self.thread, it'd have to be a list() or a set() or 
something, then you have to do a bunch of management on that. easier 
just to let the reference go out of scope and let the garbage collector 
pick it up once it stops executing. My goal here is ultimate simplicity, 
and comprehensibility in the code. Don't over-engineer! ;-)

Also, threading.enumerate was quite good I thought, I had originally 
written that as time.sleep(0.5) but then figured threading.enumerate 
would block for testing purposes without wasting extra time.

>>   class MyProtocol(Base):
>>       """Simplest possible protocol implementation to allow testing of Base."""
>> +    result = ''
>
> Do you think it would be better to initialize the result to None in Base?

No, because I do not intend self.result to actually be used in the real 
API, that was just a bit of testing instrumentation for the test suite 
only. Real API should be calling a callback at the end of the method, 
not setting some variable. if all it does is set some variable, then we 
will have to have some other thing that polls that variable. That's 
stupid. Callbacks are a way smarter thing to do here. Just think of 
self.result as a 'noop' callback for the 'noop' method.

> Also, what if you try to access result why the thread is still running?
> Should you get an exception?  Or just None?  should trying to get the result
> implicitly join the sub-thread, thus guaranteeing that the self.result is the
> return value?  Of course, that could block, but maybe that's better than
> getting a potentially incorrect value.
>
> Putting result in Base could be accompanied by a comment indicating the
> required API.  Thinking about this some more, I'm worried about race
> conditions on self.result.  What if two operations on the same plugin are
> invoked?  Let's say they both complete before the consumer reads self.result.
> Won't this cause one result to get overwritten and lost?

Yes, as you say, polling for self.result is a race condition and it'll 
be subject to differing methods trampling each other's results. Relax, 
it was just a simple way to test that the noop really did get called in 
the test suite. Real protocols will be calling callbacks, not this kind 
of thing ;-)

> I think we're now stumbling into the use cases for the defer package. :)

I don't think we've discovered a real problem just yet. ;-)