A collection of articles, ideas, and rambling from a guy who wrote some software that one time.

Thursday, September 24, 2009

Diesel: A Case Study In That Thing I Just Said

Thanks to jamwt for the shout-out on the announcement of Diesel.

Since the reaction to my reaction to tornado was so good (or at least so ... energetic), I figure I should comment on Diesel as well.  Spoiler alert: my reaction is ... largely similar, but since jamwt has been kind of nice to Twisted in the past, and didn't actually say anything mean this time, I'm somewhat reluctant to have that reaction.  Nevertheless, I swore a solemn oath to tell it like it is, keep it real, and soforth.  So I must.

Once again, I'm happy that event-driven programming is getting some love.  This time, I'm pleased that nobody is saying anything especially snarky or FUD-ish about Twisted.  I do feel like it's a little weird not to mention Twisted, or include some comparisons to Nevow or Orbited, both of which provide different, comprehensive approaches to COMET with Twisted.

(Worth noting: Orbited also originally started out using its own event-driven I/O layer, but switched to Twisted later, because Twisted is "crazy delicious".)

Diesel has many more interesting ideas at the level of async I/O than Tornado did.  I think the generator-based approach for implementing protocols is interesting and deserves some more exploration.  I'm not sold on it for every use-case, and I think the implementation might have some flaws, but it definitely has some advantages.

I'd give jamwt a hard time for not reporting issues and communicating with Twisted more before re-writing the core, but for three issues:
  1. jamwt's been around in the Twisted community for a while.  He's written a bunch of fairly deep Twisted code and he clearly knows what the framework is capable of.
  2. I've spoken with him on a number of occasions, and for all I know I might have discussed this with him.  I don't remember it, but it would be pretty embarrassing to write a big rant about how nobody talks to us only to have him paste some chat log where he explained why he was writing Diesel six months ago, and I said "oh, okay" ;-).
  3. Nobody is calling Twisted names or making vague, unsubstantiated accusations.  You're not obligated to examine Twisted, nor Nevow, nor Orbited, I just feel that you owe us some explanation if you publicly say that you tried it and found it wanting.  The tone on the Diesel announcement, in its one brief mention of Twisted, is "we tried it, but we kinda wanted to do our own thing".  So, good for them, they did their own thing, I hope they had fun.
Now, personally, I'd like to leave it at that, but there is a certain inevitable comparison that I think is going to take place.  Diesel has a nicer web page than Twisted.  They have entwittered ... twitified ... uh ... tweetened ... the project, and we haven't; we just have an old-fashioned "blog".  Diesel is smaller than Twisted, so it's easier to explain, and so the people approaching it will have a better idea of its scope.  This might give the immediate impression that it is a simpler, better, more "modern" replacement for Twisted's I/O layer, and this is not the case.  So I still feel it's important that I set the record straight.

Before I launch into my critique, I should say that I don't want to harsh on Diesel too bad. It's a neat little hack and you should go play with it.  And I feel bad pointing out problems with it, since as I mentioned above, nobody's dumping on Twisted.  So, Diesel fans, please take this in the spirit of a frank code-review, not a complaint about your behavior.

The interesting generator-munging bits could be easily adapted to run on top of Twisted's loop, which, arguably, they should have been in the first place; and the toy "hub" that they've written might be good enough for some simple applications where reliability under load is not a serious concern.  In fact, inlineCallbacks might provide a good deal of what is needed to support Diesel's programming style.  Alternately, Diesel might provide some hints as to how things like inlineCallbacks could be made more efficient.

That said, Diesel's I/O loop sucks.

It's disappointing to see the same mistakes getting made over and over again.  First and foremost: no tests.  Come on, Python community!  You can do better!  Write your damn tests first!

The #1 benefit that a brand-new I/O loop project could have over Twisted is that Twisted was written in the bad old days before everybody knew that TDD was the right way to write programs, so we don't have 100% test coverage.  But, we strive to get closer every day, while every new project decides that they don't need no stinking quality control.

Predictably, as it has no tests, Diesel's I/O layer is full of dead code, inaccurate  documentation, and unhandled errors.  Consider this gem, which I found about 30 seconds into reading the code: KqueueEventHub is documented to be "an epoll-based event hub", and its initializer defines an inner function which is never used.  I'm not going to belabor the point by enumerating all the typo bugs I found, but you may find the output of 'pyflakes diesel' interesting.

Instead of Tornado's inaccurate handling of EINTR, Diesel has no handling of EINTR, as far as I can tell.  It also doesn't handle EPERM, ENOBUFS, EMFILE, or even EAGAIN on accept().  To be fair, it has a catch-all exception handler all the way at the top of the stack, so none of these will cause instant crashes, but they will cause surprising behavior in odd situations (and possibly infinite traceback-spewing loops).

More surprisingly - I had to re-read the code about five times to make sure - it doesn't appear that sockets are ever set to be non-blocking, and EAGAIN is not handled from accept(), recv(), or send().  And yes, this can happen even if your multiplexor says your socket is ready for reading and/or writing.  The conditions are somewhat obscure, but nevertheless they do happen.  So, occasionally, Diesel will hiccup and block until some slow network client manages to send or receive some traffic.  In other words: Diesel is not really async.  It just fakes it convincingly, most of the time.

Once again, there's no way to asynchronously spawn a process, and no way to asynchronously connect a TCP client.  Sure, this looks like an asynchronous connect call, but it's misleading: it blocks on resolving the hostname, and it potentially blocks on the initial SYN/ACK/SYN+ACK exchange.  There's no asynchronous SSL support.  And no, that is not trivial.  Not to mention handling all the crazy errors that spew out of the Windows TCP stack.  And since the loop is implemented to be incompatible with Twisted, it's not obviously trivial to compatibly plug it in and get those features.

Again, I don't want to dump on Diesel here; for what it is, i.e. an experiment in how to idiomatically structure asynchronous applications, it's all right.  For that matter Twisted has its fair share of bugs too, which would be pretty easy to lay out in a similar post; you wouldn't even need to do the research yourself, just go look at our bug tracker.

But both Diesel and Tornado make the mistake of attempting to replace the years of trial-and-error, years of testing discipline, and years of portability and feature work that Twisted has accumulated with a few oversimplified, untested hacks.

What they could have done is contributed any extensions that they needed to Twisted's loop, or modifications to Twisted's packaging that would allow them to get a smaller sliver of Twisted's core to bootstrap, if that's what they needed.

My goal in pointing out all these flaws is not to illustrate any particular point about Diesel, but to reinforce the point I implicitly made in my Tornado post, which is that if you try to write a new mainloop (especially without tests) you will screw it up.  You will most likely screw it up in ways which will only surface later, under mysterious circumstances, when your servers are under load and you are under the gun for a deadline.

Or if I happen to get wind of it and write a blog post about it, of course.  Then you get to cheat a little.

It's not an indictment of Diesel that it screwed this up; everyone screws it up.  I would probably screw it up, if I didn't have Twisted sitting in front of me as a direct reference.  POSIX by itself is unreasonably subtle and difficult, but POSIX, plus the subtle variations in different platforms which implement it, plus the Windows APIs which are almost-but-not-quite-exactly-nothing-like the POSIX APIs, presents an inhuman challenge.

Hopefully Diesel will grow some tests.  Hopefully it will fix, or better yet shed, its somewhat unfortunate I/O hub.  I am hopeful that someone will follow Dustin's excellent lead (perhaps Dustin himself!) and port Diesel's API and generator system over to Twisted's I/O architecture and eliminate all these silly bugs.  Of course, it someone did that, you could use Dustin's tornado port with Diesel.

With the silly bugs from the I/O loop out of the way, the Diesel team can write tests for the more interesting pieces, and fix the bugs which aren't entirely silly :-).

8 comments:

tdavis said...

I'm completely baffled by the lack of testing, too. When I first started working with Twisted (aside: if I didn't have to make a living I'd love to spend most of my time contributing to the project!) I quickly learned the importance of tests. Nowadays, if I write some code that I didn't write tests for first I don't even bother releasing it. I feel like it's not good enough to have other people using.

I liked the concept of Tornado and of Diesel, but I stick with Twisted primarily because *I know it works* (and because it's general enough to have lots of applications, etc.). There are parts of async i/o that are obviously non-trivial to implement properly and Twisted has had a *long* time to find holes in its implementation.

That being said, Twisted does need better documentation and a nicer site wouldn't hurt things, either. Yourself and Jean Paul were a pleasure to talk to in IRC and I really wish I could put my time where my mouth is and overhaul a lot of the user-facing stuff for the project (and finish / rewrite that new HTTP client). Some day...

jamwt said...

Hi Glyph,

Just two short things.

1.) I agree with pretty much all or your criticisms. May of our documents and statements publically admit to such ("will not block (probably)"), and connect async isn't done yet. The Kqueue example is something we pull in from a contributor and haven't tested yet--I'm not sure if the contributor even considers it done. Needless to say, it's not in a release. Check out the hg tags. We're not a project that claims "head is stable" as this point, we're playing around with things.

2.) I absolutely agree about tests, and in the hacker news thread that popped up, I mentioned that this was the top priority right now, that testing is hard, and that we're probably going to need to steal from twisted trial, b/c you guys have put the effort in. We believe very strongly in tests, but frankly, I'm not disciplined enough to do TDD.

So, in summary: we're not claiming perfection out of the gate. I mean that "b" on the release version in a very real way. I won't take it off until we have a test suite in place and address some of the issues you mentioned. But we wanted to get the project out into the hands "of the people" while there was a bit of fuss already going on about async. ;-)

Thanks for the code review.

- jamwt

glyph said...

@jamwt:

Thanks for reading! I am glad you found the code review useful.

Good luck on testing. If you're interested, drop by some of our fora (the mailing list, or the IRC channel) to discuss some of our reflections on testing, since the code for trial is in many ways not the greatest example. It's still full of legacy crud from when we didn't really know how to do testing well, and there are a lot of features we want to add. Best case scenario would be for you to help us fix Trial, of course, but at least let us help you avoid our mistakes on your own thing :).

I would strongly recommend at least practicing some TDD, too. Learning how to write tests was useful for me, but practicing TDD really expanded my design skills in a bunch of surprising directions that were almost unrelated to testing. I originally felt as you do, that I wasn't disciplined enough to do it; now I feel like I'm too undisciplined not to do it ;-).

Also, even if Diesel keeps its own I/O architecture, I sincerely hope that it will at least grow a Twisted hub in the future. It seems like it would be useful to be able to write Diesel protocols that can call into existing Twisted code (for example: SSL, GUI support, process support) without too much friction. And I doubt it would be much work at all.

MichaƂ Pasternak said...

Twisted, Tornado and Diesel: The FreeBSD, NetBSD and OpenBSD of asynchronous Python networking.

Pablo said...

Thanks Glyph for continuously reminding me why porting Scrapy to other I/O loops would be a bad idea :)

Btw, I think we've done quite well with testing but we're not yet at 100% coverage, and certainly far from TDD. But I do hope to reach your TDD "mental state" sometime in the future :)

glyph said...

@Pablo:

You're certainly welcome! I actually hadn't heard of Scrapy before, but it looks like a major project using Twisted. You should really add it to http://twistedmatrix.com/trac/wiki/ProjectsUsingTwisted and http://twistedmatrix.com/trac/wiki/SuccessStories :-)

ssteinerX said...

I found this particularly interesting...

http://dieselweb.org/lib/benchmarks

The benchmark code isn't linked to from that page, maybe it's somewhere in the source package.

It'd be interesting to see where Twisted would fall in this simple "hello world" test.

David said...

Been considering whether to use Twisted or Diesel for a project, and feel free to laugh, but Diesel won out purely beacuse it's somewhat PEP8 compliant and I don't have to gripe about how ugly my code looks with googols of different naming conventions everywhere.

Find me a twisted-pep8 that doesn't make my Python look like C or Java and I'll use it. ;P (it's hard enough when the stdlib < 3.x has nothing "standard" about it...)