Merge lp:~robru/gwibber/async into lp:~barry/gwibber/py3

Hi Robert. I'm going to review the newly pushed update, which contains the
change of s/plugin/protocol/

=== modified file 'gwibber/gwibber/protocols/base.py'
--- gwibber/gwibber/protocols/base.py 2012-09-04 20:39:25 +0000
+++ gwibber/gwibber/protocols/base.py 2012-09-04 21:09:51 +0000
> @@ -19,12 +19,17 @@
> ]
>
>
> +from threading import Thread
> +
> +
> class Base:
> def __init__(self, account):
> self.account = account
>
> - def __call__(self, operation, **kws):
> - return getattr(self, operation)(**kws)
> + 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. 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)
        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.

>
> def _login(self):
> """Prevent redundant login attempts."""

=== modified file 'gwibber/gwibber/tests/test_plugins.py'
--- gwibber/gwibber/tests/test_plugins.py 2012-09-04 20:39:25 +0000
+++ gwibber/gwibber/tests/test_plugins.py 2012-09-04 21:09:51 +0000
> @@ -32,9 +32,10 @@
>
> 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?
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?

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

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? ...

On Sep 04, 2012, at 10:07 PM, Robert Bruce Park wrote:

>> 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.

Readability is probably the #1 reason why it's done this way, and especially
if it would send the line off into the rightward nether regions. But also, it
can provide a useful place to add some comments and I'm personally quite
verbose when it comes to commenting my code.

I think Python programmers often have a visceral reaction to stuffing too much
into one-liners. As for performance penalties, it will almost never make any
practical difference (and Pythonistas general attitude is that you still
shouldn't care until there's actually a problem with measurements to back it
up with. ;).
>
>> 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! ;-)

Yep, fair enough. I started experimenting with a different style and ended up
wanting to hide the thread in an outer class that gets returned. It has a
'result' property that will not return a result unless it's ready.

But if we can pretty much guarantee that the async __call__() will never
return a result in real life, then we can as you say keep it simple.

>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.

Yeah, you'd have to put the time.sleep() inside the operation to be an
accurate test of the async-ness.

Okay, I will merge the branch largely as-is (though I can't resist pissing on
the style hydrant just a little bit :). I w...

Approved and merged with some minor changes and adding the refusal to run non-public operations.

On 12-09-04 05:22 PM, Barry Warsaw wrote:
> I think Python programmers often have a visceral reaction to stuffing too much
> into one-liners. As for performance penalties, it will almost never make any
> practical difference (and Pythonistas general attitude is that you still
> shouldn't care until there's actually a problem with measurements to back it
> up with. ;).

Fair enough. My last big project was doing lots of data parsing,
particularly with expat, and I got so lost in profiling-hell that I now
obsessively do things that are known to be faster, readability be damned ;-)

>> 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! ;-)
>
> Yep, fair enough. I started experimenting with a different style and ended up
> wanting to hide the thread in an outer class that gets returned. It has a
> 'result' property that will not return a result unless it's ready.
>
> But if we can pretty much guarantee that the async __call__() will never
> return a result in real life, then we can as you say keep it simple.

I think we can make that guarantee. I'm not sure when a return value is
going to make sense really, most of the time you're going to want to
call something that results in something getting displayed on the screen.

>> 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.
>
> Yeah, you'd have to put the time.sleep() inside the operation to be an
> accurate test of the async-ness.

No, it wasn't testing asyncness, I just needed a way to say "wait for
the thread to finish in order to test that the result is correct", my
first attempt used time.sleep, then I realized that even half a second
was actually waiting much too long, so thread.enumerate offered a very
clean way to say "wait the perfect amount of time for all threads to
complete, and THEN run the test", no wasteful sleeping.

> Okay, I will merge the branch largely as-is (though I can't resist pissing on
> the style hydrant just a little bit :). I want to make it explicit that we do
> not expect, nor really support, __call__() setting any results.

Ok, fair.