Constructive Criticism

I frequently say that I'm a big fan of constructive criticism of Twisted, but I rarely get it.  People either gush about how incredibly awesomely spectacularly awesome Twisted is, or they directionlessly rant about how much it sucks, but aside from a fairly small group of regulars who file issues on the Twisted tracker, I don't hear much in between.

I caught wind of (and responded to) some blog comments of the latter type (directionless ranting) from Lakin Wecker.  After I responded, in an unusual response for someone writing such comments, he apologized and promised to do much better.  He has responded with some much more specific and potentially constructive criticism, ominously entitled "twisted part 1".

Lakin, thanks for reformulating your complaints in a more productive way.  I do think that some useful things might happen as a result of this article.  While I don't necessarily agree with it, I do care about this type of criticism.  In order to demonstrate my appreciation, I will try to make this a thorough reply.

It sounds like there are several mostly separate issues that you had here.  I'll address them one at a time.

Twisted Mail

I believe that the main issue is that the twisted.mail API is missing some convenience functionality which will allow users to quickly build SMTP responders that deal with whole messages.  This is definitely a shortcoming of twisted.mail.

However, this shortcoming is not entirely unintentional.  In general, Twisted's interfaces encourage you to write code which scales to arbitrary volumes of input.  IMessage is a thing that can receive a message, rather than a fully parsed in-memory message, because we want to encourage users to write servers that don't fall over.  If you have to handle each line as it arrives, it's less likely that you'll die if you a message bigger than the memory of the machine that is running the server.

That's not to say that there shouldn't be some additional, higher-level interface which does what you want.  Quotient, for example, uses twisted.mail, but provides a representation of a message which has all of its data written to disk first, and efficient APIs for accessing things like headers without fetching the whole message back into memory.  twisted.mail almost provides something like this itself; if you poke around in twisted.mail.maildir and twisted.mail.mail, you'll find FileMessage (an implementation of a message which writes its contents to disk) and MaildirDirdbmDomain (an implementation of IDomain which uses a directory of maildirs to deliver messages).  Not that these would not have been useful for your use case: they just show that we're happy to have higher-level stuff implemented within Twisted.

One function which might be cool to provide is something which will parse an incoming SMTP message and convert it to an email.Message.Message, then hand it off to some user code.  Even better would be to integrate this with the command-line "twistd mail" tool, such that you could easily deploy such a class as an LMTP server or something like that.

Although we don't have all the pieces you need, there is also the ever-present issue of documentation of the pieces which we do have.  Some of the code in twisted.mail might have been useful to you if its documentation had been better.  For example, you might also notice some pretty strong similarities between twisted.mail.protocols.DomainDeliveryBase.receivedHeader and your own implementation of that method.

My main point here is that fixing this is a simple matter of programming (or, in the latter case, of documenting).  I think that the best way to deal with that shortcoming is simply to submit patches to twisted.mail which add the functionality that you want.  Lots of open source projects are like this: they were driven just far enough to satisfy their implementors' use-cases.  twisted.mail is a perfectly functional and simple API if you want to build what it is designed to build.

When we're talking about "Twisted", we're typically talking about the core, and the programming model that comes with it.  When you get into the specifics of an API like twisted.mail, twisted.names, and even twisted.web (maybe even especially twisted.web) you're going to find plenty of shortcomings and areas that it don't yet do what you need.  There are some areas which are downright bad, and some which are so bad that they're embarrassing.  We need volunteers to identify the areas that are lacking and add to them.

Twisted vs. Things Which Are Not Twisted

The reason that I disagree with your conclusion that Twisted as a whole is necessarily more complex, hard to explain, too dense, unreadable (etc, etc) is that the main thing to compare it to is shared-state multithreaded socket servers, or asyncore.

Here's a good example of what makes Twisted simple, at its core:
from twisted.internet.protocol import Protocol
class Echo(Protocol):
  def dataReceived(self, data):
    self.transport.write(data)

This server supports a large number of clients.  It supports TLS.  It's cross-platform.  It supports both inbound and outbound connections.  And yet, including the import, it's only 4 lines of code.  You can write a threaded version of this which appears to be just as short, but it's pretty much impossible to do without getting a half-dozen subtleties of either a socket API or a concurrency issue wrong.

For example, your example "smtp_helper.py".  You don't provide any documentation of its concurrency properties, but the implementation of 'start' is almost certainly wrong.  For one thing, starting the same TestSMTPServer twice, or even starting two completely different TestSMTPServers at the same time, will not work.  Of course, you'd never do that, but let's say your SMTP client also used asyncore and a thread.  Now you've got a client using socket_map in the main thread and a server using socket_map in another thread.  Also, there's the fact that process_message may be called from an arbitrary thread; if it ever grew to do anything more complex than appending to a list, it would need its own serialization logic.  This isn't something that could be fixed — the entire approach is wrong, and you would need to rewrite all of your tests to work completely differently in order to fix it.  You'd need to asynchronously start both your client and your server, then have an API for letting your tests know when both of them are done.  By the time you're doing that, you're practically implementing your own mini-Twisted, along with extensions to unittest that turn it into Trial.

Ironically, you can use Twisted to fix this problem.  If you really like the API presented by the 'smtpd' module, you could write a wrapper which would make an asyncore dispatcher look like a Twisted protocol factory (or protocol), and hook asyncore into the main loop, then use 'trial' for your testing.  How exactly one would implement such a thing is beyond the scope of this post, but it's not actually that hard; just look at the relatively few methods that asyncore.dispatcher calls on self.socket and you'll probably get the idea.

I feel that the comparison of "Twisted" versus "non-Twisted" code you've presented is a bit unfair.  The Twisted example is a demonstration of utility functionality that Twisted Mail is missing, not a core idea that Twisted implements wrong.  The code it is being compared to looks simple only because critical areas of correctness that would need to be addressed in a real system (and will probably eventually need to be addressed, if the test is maintained for a long time) are being completely ignored.  The twisted example, if it fails, will fail relatively straightforwardly; the other example's failure mode will be an obscure traceback coming out of otherwise unrelated (but not thread-safe) code.

However, your subjective experience of some areas of Twisted being hard to understand and use is entirely valid.  Your detailed description of why it was difficult for you has already been useful, but I hope you will stick around and help us improve the situation for future users as well.

Trial and Testing

Perhaps the more significant issue that you discovered while you were working on this is the subtle mystery of getting Twisted to fully shut down a connection and a bound port inside a test.  This is really way too hard, and it is a problem which affects anyone who wants to use Trial for integration testing.

Although I'd really like to see this problem dealt with in a systematic way, and I'd like it to be easy as pie to write integration tests with trial, there is a reason that the issue hasn't been fixed.  As the Twisted team has been improving our testing skills, we've been finding more and more that you absolutely need good unit tests before you can really write integration tests.  Without unit tests, you don't know whether the individual pieces work, so they tend to break in surprising ways when you put them together.  In Twisted itself we are still in the process of rehabilitating a very large, and very old hodgepodge of unit, functional, and integration tests to be broken down into smaller, more coherent unit tests.  Until that process is finished, and trial has been tuned to be as good as possible for that sort of testing, integration testing isn't going to be a focus of any core developer.

I agree with the advice that you were given on IRC.  We could eliminate the particular surprise of doing a clean connection shut-down in trial, and provide a good way to do it, but you'd still face issues with your tests where the SMTP API might be scheduling timed calls or doing other things behind your back which would be difficult to monitor or shut down.  Talking to a mock message-sending implementation for starters would be a lot easier.

I can understand your concern about passing more parameters.  Luckily, this is Python: you don't necessarily need to change the interface of the system you're testing.  If you have a system, A, that depends on another system, B, to perform some of its work, you need to have a reference from A to B somewhere.  That can be passed as a parameter, imported as an object, or loaded as a module.  In Java, you'd need to change all your type declarations and do some kind of dependency injection magic, but in Python you can always cheat.  The worst case in Python, after all, is that A imports B as a module.  So, if you don't want to add any parameters, or even any attributes or methods, consider this:

# A.py
import B

def stuff():
  B.functionFromB().otherStuff()

# test_A.py
import unittest
import A
import B

class MyTest(unittest.TestCase):
  def functionFromB(self):
    result = B.functionFromB()
    # Modify the result for the test, if you like
    return result
  def setUp(self):
    A.B = self
  def tearDown(self):
    A.B = B

Some might consider this a bit gross, of course.  It might be cleaner to add a specific API for plugging in a different implementation of B.  However, it's useful to use this technique in cases — such as the one you described in your post — where you are trying to add some test coverage for an API which has already been written and you don't have control over.

I hope that digression helped, but I don't want to turn this into a screed about what you could have done better; let's consider your requirements as fixed (this needs to be an integration test) and look at what Twisted could have done better.

One thing the core team has been talking a lot about lately has been the development of verified test doubles.  We don't have a lot of them, and we need more.  For example, if you could pass a fake reactor to both your SMTP sender and receiver code, then you could manually make sure it was sending traffic at the appropriate times, to the appropriate hosts, and fail your test in sensible ways if it did something unexpected, rather than just having trial bomb out on you.  This would also let you have regression tests to make sure that your code was working with the latest version of Twisted, in case the APIs in question changed.  You wouldn't need your test to have a full, complete, clean shutdown of your SMTP connections because they would simply be garbage collected, as they would not be connected to the real reactor.  You can see an example of what this might look like in twisted.internet.task.Clock.  If someone contributed a real, documented, usable, verified test double for IReactorTCP, we would all be eternally grateful, especially if they could coalesce all the uses of the numerous half-assed attempts at it in our own test suite.

Something else we could do is write a supported factory wrapper which would allow the use of a real factory and connection in a trial test, but that would shut everything down cleanly at the connection level in tearDown.  I would personally like this a lot, but I can't promise that it would be popular with the rest of the Twisted team.  We all spend a lot of time trying to convince people to write unit tests before integration tests.  I know that I'm a little concerned that providing great integration testing support will just lead to more people being confused by weird interactions in the guts of whatever protocol they're talking to.  Eventually, however, integration tests can be useful, and I wrote the beginnings of the wrapper that I'm suggesting when I was writing tests for the AMP protocol.  You might be able to use that as an example even if Twisted doesn't provide any public APIs for that sort of thing.

Conclusions

Unfortunately there's not much I can do immediately to fix the problems that you've had, Lakin.  If someone with a similar level of Twisted experience attempts a similar task in the near future, it's likely that they'll hit the same issues.  I barely (read: didn't actually) have the time to write this blog post, and I definitely don't have the time to fix the problems I've outlined.

While there are definitely some problems here, I don't think the situation is really all that bad.  According to your post, learning enough about Twisted to do what you were doing and writing the Twisted version of this code took only 3 days.  This learning curve is not as steep as some have accused Twisted of having.  Presumably it would have taken someone already familiar with twisted.mail and trial much less time.  It didn't take me much more than 2 minutes to read and understand it :-).  As I mentioned above, your friend's threaded smtpd implementation has some pretty severe problems which might cause maintenance headaches later, whereas you were quite careful to do a proper shutdown (the trickiest thing to get right) in the Twisted version, so it is likely to be fairly robust going forward.