Code review comment for lp:~oubiwann/ubuntu-accomplishments-system/946850-twisted-app

Sorry for the delay, Duncan, I am still on vacation.

On 22 March 2012 15:22, Duncan McGreggor <email address hidden> wrote:
>>  * It looks like I needed to set my PYTHONPATH, is this the case?
>
> I was never able to duplicate the issues you had with importing python
> files from the working directory. I added a sys.path.insert(0, ".") to
> hopefully overcome that for you. If that works, you should be all set.
> If not, we'll have to do some trial and error to see what does work.
> There should be no need to set any environment variables for this to
> work, though.

Thanks, Duncan, I will test again soon.

>>  * I see you created AsyncAPI - does this literally just put these async orientated methods into a different class for the purposes of classification/naming
>
> Yup.
>
> The idea is that all the different types of API calls that are mixed
> in the Accomplishments class right now would be organized into their
> own class and then instantiated like AsyncAPI is (e.g., self.asyncapi
> = AsyncAPI()). This is simply to help with code organization,
> readability, etc.

Makes sense. :-)

>>  * It looks like each of the classes in service.py overload a set of methods in Twisted (startService, stopService etc)
>
> It doesn't overload. It simply implements the method defined by the
> interface. Each service can potentially do lots of difference things
> in its startService and stopService methods -- all different from each
> other, since each service is of a different type and has different
> responsibilities.
>
> This would be a good place to do any DBus main loop tweaks, if needed,
> for instance.
>
> (Btw, Python can't really do classic overloading, since a
> method/function can only be defined once in the same scope. However,
> you can achieve a mostly similar-seeming result with positional and
> named args and a dispatch mechanism. You could also override and then
> call the superclass's method, that also gets you something similar to
> overloading.)

So it looks like each of the different services are set up and each of
these are then used throughout the rest of the API (e.g. when we
run_scripts() this uses the ScriptRunner service). I guess the
startService/stopService pieces are then used to help them start and
stop in a twistd way (incorporating logging etc), right?

>> and then applicationFactory in app.py gets everything up and running. Is this right?
>
> Almost. applicationFactory creates an application object (with lots of
> child components) and assigns that to the variable name "application"
> in the .tac file. twistd loads the .tac file, looks for the
> "application" variable name, loads the object stored there, and fires
> of the chain of startService methods on each child object.

I saw mention of a .tac file in the twistd docs, but I didn't see it
in the code. Is there a file somewhere I should see?

> So it's twistd that gets everything up and running. applicationFactory
> just applies the configuration to various instantiations and then
> passes the application object back.

Gotcha. Btw, you showed me how to start a twistd service, how do I stop it?

Also, do you know if there is a good way to map twistd start/stop to
upstart start/stop events?

>>  * In the notes you say "all blocking calls that are going to be made in functions/methods that return deferreds should either be converted to async code that utilizes Twisted to accomplish the same thing, or be run in a thread with deferToThread" - I thought that by using inline callbacks (as my code does already) this was doing this the Twisted way and not blocking?
>
> Nope. This is one of the many pitfalls of using inlineCallbacks.
>
> inlineCallbacks is basically a wrapper for deferreds. It doesn't
> magically make code non-blocking any more than using deferreds
> directly does. There's a famous saying in the Twisted community:
>  http://www.flickr.com/photos/oubiwann/131113883/
>
> Remember:
>  * all file I/O is blocking
>  * claims of async filesystem operations are, when you get down to it,
> pretty lies
>  * as such, all (file) I/O needs to be analyzed on how it impacts a
> given program and what gets blocked and for how long
>
> This is another reason why you want to break up your long methods into
> smaller methods, especially if you're using inlineCallbacks (which you
> shouldn't be!): the logic will be easier to track, gremlins will be
> easier to see, and you can isolate async-unfriendly code more
> effectively and deal with it appropriately.

Ahhhh I see. So inlineCallbacks just magically generates the deferreds
and then continues processing. I just did some reading while I was
over here and it looks like I instead need to create a deferred for
each blocking method, and then specify the callback that processes the
deferred when it is complete. I guess I need to be careful with how I
do this as with three different services and multiple deferreds
completing at different times, I am likely to get different types of
race condition,

   Jono

--
Jono Bacon
Ubuntu Community Manager
www.ubuntu.com / www.jonobacon.org
www.identi.ca/jonobacon www.twitter.com/jonobacon