wiki:DocumentationAnalysis/Threads/AndrewBennetts

Version 1 (modified by trac, 9 years ago) (diff)

--

This is a template for experts in Twisted to use when reviewing documents as part of the DocumentationAnalysis project. See wiki:DocumentationAnalysis/DocumentList for further instructions about what to do with this template.

Things in italics get replaced with your own text or removed.

How to review: as you go to the document, think very careful about the intended user (the template asks you to describe the intended user, and eventually this information will go into the document itself in some form). Think of the type of person you would love to hand this document to. Think of the type of questions on IRC that you'd love to be able to say "go to http://blahblahblah and see section 3".

What do these users know already? What do they think they know and are likely to actually be confused about? What don't they know that you want this document to teach them?

This is the purpose of the document. Any bit of text you read and any example in the document should be able to be explained in terms of either clarifying the user's understanding or teaching them something relevant and new. (Anything that's there and not doing one of these two things is presumably serving some other purpose of the author's and you might want to consider whether this purpose is worthy of its own document. Otherwise it should be removed.)

Recall that the intended user of this review itself is a documentation writer, and your role as a reviewer is to make their job easier. Therefore address problems that it is a writer's job to fix (mistakes, missing information, irrelevant digressions) and not problems that an improved document can't help with (say, the lack of common sense, imagination and intelligence in people you have cause to interact with on a day-to-day basis).

Review details

Document expectations

Intended user

Before someone begins using this document, I'd really really like it if they knew:

  • how to use threads in Python -- they should be familiar with the threading module, and in general be aware of the need for care with inter-thread interactions because of race conditions etc.
  • the basics of working with the reactor -- starting and stopping it, what it is and what it's for.
  • what a "blocking" function is, and why it's undesirable to run them in the event loop thread
  • I'd also really like it if they knew that threads aren't needed as often as they might think ;)

Outcomes

Once someone has read this document they should understand:

  • The methods of IReactorThreads:
    • how and when to use reactor.callInThread
      • including that callInThread is not suitable for very long-running functions, because the reactor's threadpool may become filled with such requests and thus reach a state where it will never run functions passed to it.
    • how and when to use reactor.callFromThread
    • how and when to use reactor.suggestThreadPoolSize
  • That Twisted is not in general thread-safe, i.e. all Twisted code expects that it is running in the reactor's thread, so if other threads need to interact with Twisted code, they should use reactor.callFromThread to safely arrange for code to be run as part of the reactor thread.
  • Perhaps how to use twisted.python.threadpool to create your own thread pools if you don't want clog the reactor's threadpool.
  • That starting a plain Python thread is perfectly legal in Twisted, and can be a useful thing to do when a threadpool of worker threads is not appropriate.

Document review

Coverage of subject matter

This document seems to cover the following subject matter at least acceptably well:

  • that Twisted generally is not thread-safe
  • when and how to use callFromThread
  • when and how to use callInThread.
  • twisted.internet.threads.callMultipleInThread -- although I don't really see the point of this utility function (running code sequentially is what functions do already).

This document seems to be attempting to cover the following subject matter, but its coverage is flawed:

  • callFromThread is introduced before callInThread -- I wonder if the reader will be wondering "yes, but where does this other thread come from?" A possibly related problem is that the example code for callFromThread is not a self-contained, executable example, it's just a skeleton (which is probably worthwhile for understanding, but I think concrete code would also help).
  • "Running code in threads"'s example is almost a complete executable program, but is missing the reactor.run()
  • "Running code in threads" intentionally shows questionable code (using callInThread for time.sleep) for the sake of having a simple example, but either a better motivating example should be found, or it should further emphasis that this is the wrong way to do this particular task (e.g. with a docstring saying "a bad way to do reactor.callLater(2, log.msg, x)"). To be fair, the name "aSillyBlockingMethod" does try to emphasise this, but I don't think it's a clear enough signal.
  • deferToThread is mentioned, but extremely briefly, with another stub example that can't be executed, and should probably refer to the Deferred documention as well.

This document ought to be covering the following subject matter but is not:

  • the reactor threadpool is being used, but the limitations of this are not mentioned
  • the reason why Y is a better design than Z in this case
  • Foo useful library function
  • the X class of problems for which this approach needs to be modified

This document is recommending the following things that it shouldn't be recommending:

The introduction flatly contradicts the source code:

Before you start using threads, make sure you do at the start of your program:

from twisted.python import threadable
threadable.init()

vs.

def init(with_threads=1):
    """Initialize threading.

    Don't bother calling this.  If it needs to happen, it will happen.
    """

This apparently deprecated code is repeated in the examples throughout the document.

  • X out-of-date library function
  • Y sequence of calls which have been superceded by Z library

This document could be supplemented by links to these existing pre-requisitie ("you should read this first") documents: list here

This document could be supplemented by links to these existing follow-up ("now that you know X you can try Y") documents: list here

This document could be supplemented by as-yet non-existant pre-requisite ("you should read this first") documents on: list here

This document could be supplemented by links to these as-yet non-existant follow-up ("now that you know X you can try Y") documents: list here

Style

The following changes to the style of the document would make it easier to read:

  • shorter paragraphs in section 4
  • less hyperbole about the Twisted Way in the introduction

Overall summary

This document is: excellent / good, with some minor improvements to recommend / reasonable, with some pretty obvious improvements to make / better than having no documentation, but with some glaringly obvious improvements to make / so bad that it would be better to have no documentation on this