Repository: aio-libs/aiosmtpd
Pull request #55 (2017-03-27 00:32:34)

Generic hook mechanism
As an alternative to #41 here's my rough idea for a generic hook mechanism.  This is a WIP with the intention of sparking discussion.  If this is acceptable, it still needs fleshing out (more hooks), NEWS, and documentation.

I tried to make this even more generic by building the calling of handler hooks into `_handle_client()` but that had several problems.  The most complicated is that it would not allow hooks to set different statuses without changing the API of existing `SMTP.smtp_*()` methods.  Well, with avoiding exceptions for error statuses, which is one of my main motivations for this over #41 

This branch maintains API compatibility while providing a generic hook API, similar to what was previously implemented in `handle_DATA()`.  I'm hoping this will solve @Mortal issue in #47 and will be acceptable to @kozzztik as an alternative to #41 

Feedback welcome!

warsaw: One other thing this does; it makes the old `handler.process_message()` API optional, even if `handler.handle_DATA()` isn't defined.  I think that's necessary when you have handler classes with other types of hooks (e.g. `handle_RCPT()`) so they don't have to define an empty, deprecated `process_message()` hook.
Mortal: It is a bit surprising to me that handle_RCPT has to look in `envelope.rcpt_tos[-1]` (and `envelope.rcpt_options[-1]`) to find the recipient it is supposed to make a decision on.

In my case, I will call a `translate_recipient` function in my backend to resolve the recipient as a mailing list, and if this function raises an InvalidRecipient exception, I will return 550; otherwise, I will save the result of translate_recipient somewhere. @warsaw Do you intend to allow users to modify envelope.rcpt_tos in handle_RCPT? That is, would something like the following be allowed? It maintains len(envelope.rcpt_options) == 0 and replaces each incoming recipient with 0 or more outgoing recipients according to the function translate_recipient.

```python
def handle_RCPT(self, session, envelope):
    if envelope.rcpt_options.pop():
        return '555 RCPT TO parameters not recognized or not implemented'
    try:
        envelope.rcpt_tos.extend(translate_recipient(envelope.rcpt_tos.pop()))
    except InvalidRecipient as exn:
        return '550 no such mailbox %s' % (exn,)
```
warsaw: @Mortal I agree that having to dig out `rcpt_tos[-1]` is ugly.  I originally thought about passing the command args to the `handle_()` methods, but since those could be variable I decided against that, at least for the first rough cut of this branch.  Maybe we should do that (either as positional `*args` or as part of the session object?)

I do intend for handler hooks to directly modify the session and envelope objects.
Mortal: Passing the recipient alongside the session and envelope objects is also a bit unwieldy in my opinion -- I think "def handle_RCPT(self, session, envelope, recipient):" is on the long side for a method that's part of the Handler interface (especially if we additionally pass the rcpt options as an argument). Maybe I'm biased since my use-case does not need the session object at all in handle_RCPT or handle_DATA, but I think forcing every Handler implementation to take more than self + two arguments in handle_RCPT is bad.

It would be really nice if the session and envelope objects were available on self and the interface was "def handle_RCPT(self, recipient):". However, as far as I understand, aiosmtpd Handlers are instantiated once and reused for each SMTP client, right? That kind of rules out any client-specific state on the Handler.

Alternatively, could the session be stored on the envelope, or vice-versa? Then we would just have "def handle_RCPT(self, session, recipient, opts):".

A separate question: What are the use-cases for RCPT options? I don't know what they are used for, and the current implementation sends a 555 error if any are passed. Could RCPT options be opt-in via, say, a class variable on the Handler?
warsaw: Note that the generic API I am thinking about is `def handle_*(self, session, envelope, arg)` since all the `smtp_*()` command take a single argument.  I have to think about whether that's a general case for SMTP/LMTP or just an accident of implementation.  (IOW, should it be `*args` for future proofing?)

I don't think that's terribly unwieldy, and it is easier to remember the API when all methods take the same arguments.

It's definitely possible that handler methods won't need the session or envelope, but they *might* so I think we should pass them in.  As mentioned, it should be possible for handler methods to directly modify these objects.

In @kozzztik 's original branch we talked about making the envelope and session the same object, but they have different lifetimes so they need to be separate.  I don't think it really buys that much to put one attribute on the other.  If it just saves a little typing in the handler methods, it makes the implementation more complex (because you have to manage the separate lifetimes).  A similar argument applies to putting the session or envelope on the handler instance, since as you observe, it's created once and reused.  (That's not something I want to change.)

We don't need to pass in `rcpt_options` since these are available on the envelope.  The format for `RCPT TO` additional parameters are defined in the ESMTP standard.  We should probably do a better job of allowing subclasses of `SMTP` and/or handlers to handle `RCPT TO` options without having to cargo cult the entire `smtp_RCPT()` method in a subclass.  If that's of interest, let's discuss in a separate issue/pr.
Mortal: For what it's worth, I just looked over [Wikipedia's list of published SMTP service extensions](https://en.wikipedia.org/wiki/Extended_SMTP#Extensions), and the only RFC I found that adds RCPT options is [3461 Delivery Status Notifications](https://tools.ietf.org/html/rfc3461) (which adds EHLO DSN along with two RCPT options and two MAIL options). While aiosmtpd should certainly support handlers/subclasses that want to implement this RFC, I don't think RCPT options will be in high demand, so only having access to them on the envelope is fine.

Keeping session and envelope separate, and passing both to the `handle_*` methods, is fine by me.

From my reading of [RFC 2821](https://tools.ietf.org/html/rfc2821) it seems to me that all commands take at most one argument, except RSET and QUIT which never take an argument. If we want to have handle_RSET or handle_QUIT in the Handler interface, we would have to decide between an interface of "def handle_RSET(self, session, envelope, _arg=None):" or "def handle_RSET(self, session, envelope):".

* EHLO/HELO - required argument
* MAIL/RCPT - required argument + optional params
* RSET/QUIT - no argument
* VRFY/EXPN - required argument
* HELP/NOOP - optional argument
kozzztik: Hi!
I just added a draft for a custom RCPT and MAIL methods, I`m think you would like it. In that case I added additional logic that based on presence of method on handler. I`m think that, last draft of RCPT handlers in #57 is best one. But it strongly sets no place for compromise between duck typing and base handler class.
warsaw: @kozzztik @Mortal I've updated this branch based on the discussions in #57.  I think I've taken the most important ideas from the previous hook branches, so please let me know if this will serve your purposes.

If so, then I'll update the docs and we'll land this as the hooks API.
warsaw: Can we ditch Python 3.4 yet? ;)
warsaw: The tests are hanging in Python 3.4 on Travis.  I don't know why though because they pass fine locally on Python 3.4 from git head.
warsaw: Okay, this branch now contains the changes discussed among the various proposals.  I'm liking it!  @kozzztik @Mortal what do you think?
Mortal: I like the API. I suggest changing the first paragraph of the section on handlers to:

```
Handlers can implement hooks that get called during the ``SMTP`` dialog, or in
exceptional cases.  These *handler hooks* are all called asynchronously and
they *must* return a status string such as ``'250 OK'``.  All handlers are
optional and have certain default behaviors when omitted, but when handlers are
not omitted they may have additional responsibilities, as described below.
```

This fixes a typo ("a hooks") and tries to clarify two phrases that confused me on a first reading: "since the default statuses are not returned when a hook is defined" and "to replace default behavior". I think it is important to highlight that all handler hooks are optional.
warsaw: @Mortal Thanks for the feedback; I'll look at your suggestions in more detail a little later.  I would love to get @kozzztik 's opinion of this hook API.  6 weeks until Pycon! :)
warsaw: On Apr 01, 2017, at 06:06 PM, Mathias Rav wrote:

>This fixes a typo ("a hooks") and tries to clarify two phrases that confused
>me on a first reading: "since the default statuses are not returned when a
>hook is defined" and "to replace default behavior". I think it is important
>to highlight that all handler hooks are optional.

Thanks, that's an improvement.  I made a few modifications since the ``SMTP``
class *does* require a handler, but it doesn't require any specific hooks.
Here's the text I'll use:

```
Handlers can implement hooks that get called during the SMTP dialog, or in
exceptional cases.  These *handler hooks* are all called asynchronously and
they *must* return a status string, such as ``'250 OK'``.  All handler hooks
are optional and certain default behaviors are carried out by the ``SMTP``
class when a hook is omitted, but when handler hooks are defined, they may
have additional responsibilities, as described below.
```

(One other convention I'm trying to keep; when I'm talking about the SMTP
protocol, no markup, but when I'm talking about the ``SMTP`` class, markup.)

warsaw: On Apr 01, 2017, at 06:24 PM, Mathias Rav wrote:

>This is a run-on sentence, the ", the" should be replaced with ". The" in the
>description of all "handle_*" methods.

+1

>+``handle_EHLO(server, session, envelope, hostname)``
>+    Called during ``EHLO``, the ``hostname`` argument is the host name given
>+    by the client in the ``EHLO`` command.  If implemented, this hook must
>+    also set the ``session.host_name`` attribute.  This hook may push
>+    additional ``250-<command>`` responses to the client by yielding from
>+    ``server.push(status)``.
>
>I suggest adding " before returning ``250 OK`` as the final response." at the
>end to clarify that `handle_EHLO` should still return the final response.

+1

>
>> +  
>+``handle_RCPT(server, session, envelope, address, rcpt_options)``
>+    Called during ``RCPT TO``, the ``address`` argument is the parsed email
>+    address given by the client in the ``RCPT TO`` command, and
>+    ``rcpt_options`` are any additional ESMTP recipient options providing by
>+    the client.  If implemented, this hook should append the address to
>+    ``envelope.rcpt_tos`` and may extend ``envelope.rcpt_options`` (both of
>+    which are always Python lists).
>+
>+``handle_RSET(server, session, envelope)``
>+    Called during ``RSET``.
>+
>+``handle_DATA(session, envelope)``
>+    Called during ``DATA`` after most processing of the data has occurred.
>+    Hooks can inspect or change the converted data by looking at
>+    ``envelope.content``.
>
>I think "after most processing of the data has occurred" sounds very vague. I
>suggest changing the first sentence to "Called during ``DATA`` when the
>entire message has been received."
>
>Also, in what sense is `envelope.content` the *converted* data? Wouldn't it
>be more precise to call this the *incoming* data?

In thinking about this a little bit, I wonder if we shouldn't leave it to the
`handle_DATA()` hook to set `envelope.content`?  This would mean a slight
change to the signature of the method.  Right now it just takes the standard
`session` and `envelope` arguments, but we *could* change that to take:

```
def handle_DATA(session, envelope, bytes_content, string_content=None)
```

`bytes_content` would obviously always be bytes, but it isn't exactly the raw
on-the-wire content.  It's normalized as described in this comment:

```
        # Remove extraneous carriage returns and de-transparency
        # according to RFC 5321, Section 4.5.2.
```

`string_content` *may* be provided if `SMTP` was instantiated with
`decode_data=True`.  Otherwise, it will be None (or not passed?)

I'm not sure this is an improvement; we could just set both on the `envelope`
object and just do a better job of documenting exactly what is passed, as you
suggest.

warsaw: Heh, "email replies don't support markdown".
kozzztik: @warsaw, i still don`t love duck typing, but this PR sure, looks much better then we have now! Lets land it, and I will add correct Proxy handling implementation with this. May be this will be final point in search of compromise.
warsaw: @kozzztik Great, thanks for the feedback!

I plan to make one other minor change.  I've thought about the envelope content, and my thinking is that we should continue to *not* pass that as an argument to `handle_DATA()`.  We'll still put that on the `envelope` object, but I am also going to add an attribute to the envelope which will be the unconverted bytes in the case where `decode_data=True`.

I will make that change and do a final docs pass, then I'll merge this branch once the CI passes, and we can move on to the other PRs and issues.
kozzztik: Is there a case "decode_data" covers? I`m think there are two main use cases:
1. Get binary data to store or transmit as-is, add headers, but not invade content (only binary data needed)
2. Parse email using email package (it will decode data itself)
I have no idea why someone may need decoded data. Although, it is heavy operation for CPU and memory usage (and more if we will save both strings in memory) as mails may be huge. 
Anyway, all that decode_data do is str(utf8), do we need special parameter for that?
P.S. I dream about some kind of data streaming, so you don`t have to store original data in memory. But it will be great challenge.
kozzztik: Thats not blocks PR, just a separate question )
warsaw: On Apr 03, 2017, at 09:44 AM, Konstantin Volkov wrote:

>Is there a case "decode_data" covers?

Just to point out that this is inherited from smtpd.py.  I'm not sure how
useful it is to keep, and I don't remember what the rationale was for adding
it to smtpd.py.  I suspect it was to better work with the old mimelib/email
package in Python 2, but Python 3's email package can parse bytes just fine.

>I have no idea why someone may need decoded data. Although, it is heavy
>operation for CPU and memory usage (and more if we will save both strings in
>memory) as mails may be huge.  Anyway, all that decode_data do is str(utf8),
>do we need special parameter for that?

You're right that the main difference is that we just utf-8 decode the bytes.
The only other functional effect of setting that flag is to *not* push
250-8BITMIME for EHLO response.

If we wanted to remove it, we'd need to do so in a backward compatible way.
Perhaps we should deprecate it for 1.0 (but let's open a new issue/PR for
that).

>P.S. I dream about some kind of data streaming, so you don`t have to store
>original data in memory. But it will be great challenge.

This is something we've talked about in the email-sig too.  Lots of folks want
that, but so far we don't have any viable code or proposals to add such an API.
It would be cool if a streaming API were added to make both aiosmtpd and the
email package work better together.

kozzztik: > This is something we've talked about in the email-sig too. Lots of folks want that, but so far we don't have any viable code or proposals to add such an API. It would be cool if a streaming API were added to make both aiosmtpd and the email package work better together.
Yep, that`s a place a wont to work on in May. We have some performance and memory usage issues on this stage in our project, and that is our bottleneck, so it will be very interesting to deal with it.
warsaw: Looks like Travis is slow or hung.

Patch:
diff --git a/aiosmtpd/docs/NEWS.rst b/aiosmtpd/docs/NEWS.rst
index 2597329..7312bd0 100644
--- a/aiosmtpd/docs/NEWS.rst
+++ b/aiosmtpd/docs/NEWS.rst
@@ -4,8 +4,11 @@
 
 1.0a5 (201X-XX-XX)
 ==================
-* Deprecate handler `process_message()` methods.  Use the new asynchronous
-  `handle_DATA()` methods, which take a session and an envelope object.
+* A new handler hook API has been added which provides more flexibility but
+  requires more responsibility (e.g. hooks must return a string status).
+  Deprecate ``SMTP.ehlo_hook()`` and ``SMTP.rset_hook()``.
+* Deprecate handler ``process_message()`` methods.  Use the new asynchronous
+  ``handle_DATA()`` methods, which take a session and an envelope object.
 * Added the ``STARTTLS`` extension.  Given by Konstantin Volkov.
 * Minor changes to the way the ``Debugging`` handler prints ``mail_options``
   and ``rcpt_options`` (although the latter is still not support in ``SMTP``).
diff --git a/aiosmtpd/docs/smtp.rst b/aiosmtpd/docs/smtp.rst
index 2d8a6e5..a7626b6 100644
--- a/aiosmtpd/docs/smtp.rst
+++ b/aiosmtpd/docs/smtp.rst
@@ -81,6 +81,9 @@ incremented.
 Server hooks
 ============
 
+.. warning:: These methods are deprecated.  :ref:`handler hooks <hooks>`
+             instead.
+
 The ``SMTP`` server class also implements some hooks which your subclass can
 override to provide additional responses.
 
@@ -104,31 +107,92 @@ override to provide additional responses.
 Handler hooks
 =============
 
-Handlers can implement a few hooks that get called during the ``SMTP`` dialog,
-or in exceptional cases.  More hooks may be added in the future, but for now
-the following handler hooks are defined:
+Handlers can implement hooks that get called during the SMTP dialog, or in
+exceptional cases.  These *handler hooks* are all called asynchronously and
+they *must* return a status string, such as ``'250 OK'``.  All handler hooks
+are optional and certain default behaviors are carried out by the ``SMTP``
+class when a hook is omitted, but when handler hooks are defined, they may
+have additional responsibilities, as described below.
+
+All handler hooks take at least three arguments, the ``SMTP`` server instance,
+:ref:`a session instance, and an envelope instance <sessions_and_envelopes>`.
+Some methods take additional arguments.
+
+The following hooks are currently defined:
+
+``handle_HELO(server, session, envelope, hostname)``
+    Called during ``HELO``.  The ``hostname`` argument is the host name given
+    by the client in the ``HELO`` command.  If implemented, this hook must
+    also set the ``session.host_name`` attribute.
+
+``handle_EHLO(server, session, envelope, hostname)``
+    Called during ``EHLO``.  The ``hostname`` argument is the host name given
+    by the client in the ``EHLO`` command.  If implemented, this hook must
+    also set the ``session.host_name`` attribute.  This hook may push
+    additional ``250-<command>`` responses to the client by yielding from
+    ``server.push(status)`` before returning ``250 OK`` as the final response.
+
+``handle_NOOP(server, session, envelope)``
+    Called during ``NOOP``.
+
+``handle_QUIT(server, session, envelope)``
+    Called during ``QUIT``.
+
+``handle_VRFY(server, session, envelope, address)``
+    Called during ``VRFY``.  The ``address`` argument is the parsed email
+    address given by the client in the ``VRFY`` command.
+
+``handle_MAIL(server, session, envelope, address, mail_options)``
+    Called during ``MAIL FROM``.  The ``address`` argument is the parsed email
+    address given by the client in the ``MAIL FROM`` command, and
+    ``mail_options`` are any additional ESMTP mail options providing by the
+    client.  If implemented, this hook must also set the
+    ``envelope.mail_from`` attribute and it may extend
+    ``envelope.mail_options`` (which is always a Python list).
+
+``handle_RCPT(server, session, envelope, address, rcpt_options)``
+    Called during ``RCPT TO``.  The ``address`` argument is the parsed email
+    address given by the client in the ``RCPT TO`` command, and
+    ``rcpt_options`` are any additional ESMTP recipient options providing by
+    the client.  If implemented, this hook should append the address to
+    ``envelope.rcpt_tos`` and may extend ``envelope.rcpt_options`` (both of
+    which are always Python lists).
+
+``handle_RSET(server, session, envelope)``
+    Called during ``RSET``.
+
+``handle_DATA(session, envelope)``
+    Called during ``DATA`` after the entire message (`"SMTP content"
+    <https://tools.ietf.org/html/rfc5321#section-2.3.9>`_ as described in
+    RFC 5321) has been received.  The content is available on the ``envelope``
+    object, but the values are dependent on whether the ``SMTP`` class was
+    instantiated with ``decode_data=False`` (the default) or
+    ``decode_data=True``.  In the former case, both ``envelope.content`` and
+    ``envelope.original_content`` will be the content bytes (normalized
+    according to the transparency rules in `RFC 5321, $4.5.2
+    <https://tools.ietf.org/html/rfc5321#section-4.5.2>`_).  In the latter
+    case, ``envelope.original_content`` will be the normalized bytes, but
+    ``envelope.content`` will be the UTF-8 decoded string of the original
+    content.
+
+In addition to the SMTP command hooks, the following hooks can also be
+implemented by handlers.  They have a different signature and don't need to
+return any status.  Both methods are called synchronously.
 
 ``handle_tls_handshake(session)``
-    (*optional*, *synchronous*) If implemented, and if SSL is supported, this
-    handler method gets called during the TLS handshake phase of
-    ``connection_made()``.  It should return a boolean which specifies whether
-    the handshake failed or not.  ``session`` is an instance of the Session_
-    object.
+    If implemented, and if SSL is supported, this handler method gets called
+    during the TLS handshake phase of ``connection_made()``.  It should return
+    a boolean which specifies whether the handshake failed or not.
 
 ``handle_exception(error)``
-    (*optional*, *synchronous*) If implemented, this method is called when any
-    error occurs during the handling of a connection (e.g. if an
-    ``smtp_COMMAND()`` command raises an exception).  The exception object is
-    passed in.  Note that as part of the ``SMTP`` dialog, if an exception
-    occurs, a 500 code will be returned to the client.
+    If implemented, this method is called when any error occurs during the
+    handling of a connection (e.g. if an ``smtp_<command>()`` method raises an
+    exception).  The exception object is passed in.  Note that as part of the
+    ``SMTP`` dialog, if an exception occurs, a 500 code will be returned to
+    the client.
 
-``handle_DATA(session, envelope)``
-    (*required*, *asynchronous*) This method is called on the handler so that
-    it can process the incoming ``DATA`` bytes.  The ``session`` and
-    ``envelope`` arguments are described below.  It returns the status string
-    to pass back to the client.  If status is not given (e.g. it is None),
-    then the string ``"250 OK"`` is used as the status.
 
+.. _sessions_and_envelopes:
 
 Sessions and envelopes
 ======================
diff --git a/aiosmtpd/handlers.py b/aiosmtpd/handlers.py
index b374562..d1fd871 100644
--- a/aiosmtpd/handlers.py
+++ b/aiosmtpd/handlers.py
@@ -65,7 +65,7 @@ class Debugging:
             print(line, file=self.stream)
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         print('---------- MESSAGE FOLLOWS ----------', file=self.stream)
         # Yes, actually test for truthiness since it's possible for either the
         # keywords to be missing, or for their values to be empty lists.
@@ -82,6 +82,7 @@ class Debugging:
             print(file=self.stream)
         self._print_message_content(session.peer, envelope.content)
         print('------------ END MESSAGE ------------', file=self.stream)
+        return '250 OK'
 
 
 @public
@@ -91,7 +92,7 @@ class Proxy:
         self._port = remote_port
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         lines = envelope.content.splitlines(keepends=True)
         # Look for the last header
         i = 0
@@ -106,6 +107,7 @@ class Proxy:
         refused = self._deliver(envelope.mail_from, envelope.rcpt_tos, data)
         # TBD: what to do with refused addresses?
         log.info('we got some refusals: %s', refused)
+        return '250 OK'
 
     def _deliver(self, mail_from, rcpt_tos, data):
         refused = {}
@@ -139,10 +141,6 @@ class Sink:
             parser.error('Sink handler does not accept arguments')
         return cls()
 
-    @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
-        pass                                        # pragma: nocover
-
 
 @public
 class Message:
@@ -150,9 +148,10 @@ class Message:
         self.message_class = message_class
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         envelope = self.prepare_message(session, envelope)
         self.handle_message(envelope)
+        return '250 OK'
 
     def prepare_message(self, session, envelope):
         # If the server was created with decode_data True, then data will be a
@@ -180,9 +179,10 @@ class AsyncMessage(Message):
         self.loop = loop or asyncio.get_event_loop()
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         message = self.prepare_message(session, envelope)
         yield from self.handle_message(message)
+        return '250 OK'
 
     @asyncio.coroutine
     def handle_message(self, message):
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index 825bee3..e2695de 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -23,11 +23,13 @@ __ident__ = 'Python SMTP {}'.format(__version__)
 log = logging.getLogger('mail.log')
 
 
-NEWLINE = '\n'
 DATA_SIZE_DEFAULT = 33554432
 EMPTYBYTES = b''
+NEWLINE = '\n'
+MISSING = object()
 
 
+@public
 class Session:
     def __init__(self, loop):
         self.peer = None
@@ -37,10 +39,11 @@ class Session:
         self.loop = loop
 
 
+@public
 class Envelope:
     def __init__(self):
         self.mail_from = None
-        self.mail_options = None
+        self.mail_options = []
         self.content = None
         self.rcpt_tos = []
         self.rcpt_options = []
@@ -164,9 +167,9 @@ class SMTP(asyncio.StreamReaderProtocol):
         self._set_post_data_state()
 
     @asyncio.coroutine
-    def push(self, msg):
+    def push(self, status):
         response = bytes(
-            msg + '\r\n', 'utf-8' if self.require_SMTPUTF8 else 'ascii')
+            status + '\r\n', 'utf-8' if self.require_SMTPUTF8 else 'ascii')
         self._writer.write(response)
         log.debug(response)
         yield from self._writer.drain()
@@ -176,6 +179,14 @@ class SMTP(asyncio.StreamReaderProtocol):
         if hasattr(self.event_handler, 'handle_exception'):
             yield from self.event_handler.handle_exception(error)
 
+    @asyncio.coroutine
+    def _call_handler_hook(self, command, *args):
+        hook = getattr(self.event_handler, 'handle_' + command, None)
+        if hook is None:
+            return MISSING
+        status = yield from hook(self, self.session, self.envelope, *args)
+        return status
+
     @asyncio.coroutine
     def _handle_client(self):
         log.info('handling connection')
@@ -217,7 +228,7 @@ class SMTP(asyncio.StreamReaderProtocol):
                         '530 Must issue a STARTTLS command first')
                     continue
                 method = getattr(self, 'smtp_' + command, None)
-                if not method:
+                if method is None:
                     yield from self.push(
                         '500 Error: command "%s" not recognized' % command)
                     continue
@@ -239,31 +250,24 @@ class SMTP(asyncio.StreamReaderProtocol):
             yield from self.push('503 Duplicate HELO/EHLO')
             return
         self._set_rset_state()
-        self.session.host_name = hostname
         self.session.extended_smtp = False
-        yield from self.push('250 %s' % self.hostname)
+        status = yield from self._call_handler_hook('HELO', hostname)
+        if status is MISSING:
+            self.session.host_name = hostname
+            status = '250 {}'.format(self.hostname)
+        yield from self.push(status)
 
     @asyncio.coroutine
-    def ehlo_hook(self):
-        """Allow subclasses to extend EHLO responses.
-
-        This hook is called just before the final, non-continuing
-        `250 HELP` response.  Subclasses can add additional `250-<cmd>`
-        responses for custom behavior.
-        """
-        pass
-
-    @asyncio.coroutine
-    def smtp_EHLO(self, arg):
-        if not arg:
+    def smtp_EHLO(self, hostname):
+        if not hostname:
             yield from self.push('501 Syntax: EHLO hostname')
             return
-        # See issue #21783 for a discussion of this behavior.
+        # See https://bugs.python.org/issue21783 for a discussion of this
+        # behavior.
         if self.session.host_name:
             yield from self.push('503 Duplicate HELO/EHLO')
             return
         self._set_rset_state()
-        self.session.host_name = arg
         self.session.extended_smtp = True
         yield from self.push('250-%s' % self.hostname)
         if self.data_size_limit:
@@ -278,22 +282,31 @@ class SMTP(asyncio.StreamReaderProtocol):
                 not self._tls_protocol and
                 _has_ssl):                        # pragma: nossl
             yield from self.push('250-STARTTLS')
-        yield from self.ehlo_hook()
-        yield from self.push('250 HELP')
+        if hasattr(self, 'ehlo_hook'):
+            warn('Use handler.handle_EHLO() instead of .ehlo_hook()',
+                 DeprecationWarning)
+            yield from self.ehlo_hook()
+        status = yield from self._call_handler_hook('EHLO', hostname)
+        if status is MISSING:
+            self.session.host_name = hostname
+            status = '250 HELP'
+        yield from self.push(status)
 
     @asyncio.coroutine
     def smtp_NOOP(self, arg):
         if arg:
             yield from self.push('501 Syntax: NOOP')
         else:
-            yield from self.push('250 OK')
+            status = yield from self._call_handler_hook('NOOP')
+            yield from self.push('250 OK' if status is MISSING else status)
 
     @asyncio.coroutine
     def smtp_QUIT(self, arg):
         if arg:
             yield from self.push('501 Syntax: QUIT')
         else:
-            yield from self.push('221 Bye')
+            status = yield from self._call_handler_hook('QUIT')
+            yield from self.push('221 Bye' if status is MISSING else status)
             self._handler_coroutine.cancel()
             self.transport.close()
 
@@ -394,12 +407,14 @@ class SMTP(asyncio.StreamReaderProtocol):
                 address, params = self._getaddr(arg)
             except HeaderParseError:
                 address = None
-            if address:
+            if address is None:
+                yield from self.push('502 Could not VRFY %s' % arg)
+            else:
+                status = yield from self._call_handler_hook('VRFY', address)
                 yield from self.push(
                     '252 Cannot VRFY user, but will accept message '
-                    'and attempt delivery')
-            else:
-                yield from self.push('502 Could not VRFY %s' % arg)
+                    'and attempt delivery'
+                    if status is MISSING else status)
         else:
             yield from self.push('501 Syntax: VRFY <address>')
 
@@ -454,14 +469,18 @@ class SMTP(asyncio.StreamReaderProtocol):
                     '552 Error: message size exceeds fixed maximum message '
                     'size')
                 return
-        if len(params.keys()) > 0:
+        if len(params) > 0:
             yield from self.push(
                 '555 MAIL FROM parameters not recognized or not implemented')
             return
-        self.envelope.mail_from = address
-        self.envelope.mail_options = mail_options
+        status = yield from self._call_handler_hook(
+            'MAIL', address, mail_options)
+        if status is MISSING:
+            self.envelope.mail_from = address
+            self.envelope.mail_options.extend(mail_options)
+            status = '250 OK'
         log.info('sender: %s', address)
-        yield from self.push('250 OK')
+        yield from self.push(status)
 
     @asyncio.coroutine
     def smtp_RCPT(self, arg):
@@ -496,15 +515,14 @@ class SMTP(asyncio.StreamReaderProtocol):
             yield from self.push(
                 '555 RCPT TO parameters not recognized or not implemented')
             return
-        self.envelope.rcpt_tos.append(address)
-        self.envelope.rcpt_options.append(rcpt_options)
+        status = yield from self._call_handler_hook(
+            'RCPT', address, rcpt_options)
+        if status is MISSING:
+            self.envelope.rcpt_tos.append(address)
+            self.envelope.rcpt_options.extend(rcpt_options)
+            status = '250 OK'
         log.info('recip: %s', address)
-        yield from self.push('250 OK')
-
-    @asyncio.coroutine
-    def rset_hook(self):
-        """Allow subclasses to hook into the RSET command."""
-        pass
+        yield from self.push(status)
 
     @asyncio.coroutine
     def smtp_RSET(self, arg):
@@ -512,8 +530,12 @@ class SMTP(asyncio.StreamReaderProtocol):
             yield from self.push('501 Syntax: RSET')
             return
         self._set_rset_state()
-        yield from self.rset_hook()
-        yield from self.push('250 OK')
+        if hasattr(self, 'rset_hook'):
+            warn('Use handler.handle_RSET() instead of .rset_hook()',
+                 DeprecationWarning)
+            yield from self.rset_hook()
+        status = yield from self._call_handler_hook('RSET')
+        yield from self.push('250 OK' if status is MISSING else status)
 
     @asyncio.coroutine
     def smtp_DATA(self, arg):
@@ -553,30 +575,37 @@ class SMTP(asyncio.StreamReaderProtocol):
             text = data[i]
             if text and text[:1] == b'.':
                 data[i] = text[1:]
-        received_data = EMPTYBYTES.join(data)
+        content = original_content = EMPTYBYTES.join(data)
         if self._decode_data:
-            received_data = received_data.decode('utf-8')
-        self.envelope.content = received_data
+            content = original_content.decode('utf-8')
+        self.envelope.content = content
+        self.envelope.original_content = original_content
         # Call the new API first if it's implemented.
         if hasattr(self.event_handler, 'handle_DATA'):
-            args = (self.session, self.envelope)
-            status = yield from self.event_handler.handle_DATA(*args)
+            status = yield from self._call_handler_hook('DATA')
         else:
-            warn('Use handler.handle_DATA instead of .process_message()',
-                 DeprecationWarning)
-            args = (self.session.peer, self.envelope.mail_from,
-                    self.envelope.rcpt_tos, self.envelope.content)
-            if asyncio.iscoroutinefunction(self.event_handler.process_message):
-                status = yield from self.event_handler.process_message(*args)
-            else:
-                status = self.event_handler.process_message(*args)
+            # Backward compatibility.
+            status = MISSING
+            if hasattr(self.event_handler, 'process_message'):
+                warn('Use handler.handle_DATA() instead of .process_message()',
+                     DeprecationWarning)
+                args = (self.session.peer, self.envelope.mail_from,
+                        self.envelope.rcpt_tos, self.envelope.content)
+                if asyncio.iscoroutinefunction(
+                        self.event_handler.process_message):
+                    status = yield from self.event_handler.process_message(
+                        *args)
+                else:
+                    status = self.event_handler.process_message(*args)
+                # The deprecated API can return None which means, return the
+                # default status.  Don't worry about coverage for this case as
+                # it's a deprecated API that will go away after 1.0.
+                if status is None:                  # pragma: nocover
+                    status = MISSING
         self._set_post_data_state()
-        if status:
-            yield from self.push(status)
-        else:
-            yield from self.push('250 OK')
+        yield from self.push('250 OK' if status is MISSING else status)
 
-    # Commands that have not been implemented
+    # Commands that have not been implemented.
     @asyncio.coroutine
     def smtp_EXPN(self, arg):
         yield from self.push('502 EXPN not implemented')


Test patch:
diff --git a/aiosmtpd/tests/test_handlers.py b/aiosmtpd/tests/test_handlers.py
index e433810..6c7ba8c 100644
--- a/aiosmtpd/tests/test_handlers.py
+++ b/aiosmtpd/tests/test_handlers.py
@@ -4,14 +4,13 @@ import asyncio
 import unittest
 
 from aiosmtpd.controller import Controller
-from aiosmtpd.handlers import (
-    AsyncMessage, Debugging, Mailbox, Message, Proxy, Sink)
+from aiosmtpd.handlers import AsyncMessage, Debugging, Mailbox, Proxy, Sink
 from aiosmtpd.smtp import SMTP as Server
 from contextlib import ExitStack
 from io import StringIO
 from mailbox import Maildir
 from operator import itemgetter
-from smtplib import SMTP, SMTPRecipientsRefused
+from smtplib import SMTP, SMTPDataError, SMTPRecipientsRefused
 from tempfile import TemporaryDirectory
 from unittest.mock import call, patch
 
@@ -152,22 +151,25 @@ Testing
 """.format(peer))
 
 
-class TestMessage(unittest.TestCase):
-    def setUp(self):
-        self.handled_message = None
+class DataHandler:
+    def __init__(self):
+        self.content = None
+        self.original_content = None
 
-        class MessageHandler(Message):
-            def handle_message(handler_self, message):
-                self.handled_message = message
+    @asyncio.coroutine
+    def handle_DATA(self, server, session, envelope):
+        self.content = envelope.content
+        self.original_content = envelope.original_content
+        return '250 OK'
 
-        self.handler = MessageHandler()
 
+class TestMessage(unittest.TestCase):
     def test_message(self):
-        # In this test, the message data comes in as bytes.
-        controller = Controller(self.handler)
+        # In this test, the message content comes in as a bytes.
+        handler = DataHandler()
+        controller = Controller(handler)
         controller.start()
         self.addCleanup(controller.stop)
-
         with SMTP(controller.hostname, controller.port) as client:
             client.sendmail('anne@example.com', ['bart@example.com'], """\
 From: Anne Person <anne@example.com>
@@ -177,22 +179,17 @@ Message-ID: <ant>
 
 Testing
 """)
-        self.assertEqual(self.handled_message['subject'], 'A test')
-        self.assertEqual(self.handled_message['message-id'], '<ant>')
-        self.assertIsNotNone(self.handled_message['X-Peer'])
-        self.assertEqual(
-            self.handled_message['X-MailFrom'], 'anne@example.com')
-        self.assertEqual(self.handled_message['X-RcptTo'], 'bart@example.com')
+        # The content is not converted, so it's bytes.
+        self.assertEqual(handler.content, handler.original_content)
+        self.assertIsInstance(handler.content, bytes)
+        self.assertIsInstance(handler.original_content, bytes)
 
     def test_message_decoded(self):
-        # With a server that decodes the data, the messages come in as
-        # strings.  There's no difference in the message seen by the
-        # handler's handle_message() method, but internally this gives full
-        # coverage.
-        controller = UTF8Controller(self.handler)
+        # In this test, the message content comes in as a string.
+        handler = DataHandler()
+        controller = UTF8Controller(handler)
         controller.start()
         self.addCleanup(controller.stop)
-
         with SMTP(controller.hostname, controller.port) as client:
             client.sendmail('anne@example.com', ['bart@example.com'], """\
 From: Anne Person <anne@example.com>
@@ -202,12 +199,9 @@ Message-ID: <ant>
 
 Testing
 """)
-        self.assertEqual(self.handled_message['subject'], 'A test')
-        self.assertEqual(self.handled_message['message-id'], '<ant>')
-        self.assertIsNotNone(self.handled_message['X-Peer'])
-        self.assertEqual(
-            self.handled_message['X-MailFrom'], 'anne@example.com')
-        self.assertEqual(self.handled_message['X-RcptTo'], 'bart@example.com')
+        self.assertNotEqual(handler.content, handler.original_content)
+        self.assertIsInstance(handler.content, str)
+        self.assertIsInstance(handler.original_content, bytes)
 
 
 class TestAsyncMessage(unittest.TestCase):
@@ -481,6 +475,124 @@ Testing
                 )
 
 
+class HELOHandler:
+    @asyncio.coroutine
+    def handle_HELO(self, server, session, envelope, hostname):
+        return '250 geddy.example.com'
+
+
+class EHLOHandler:
+    @asyncio.coroutine
+    def handle_EHLO(self, server, session, envelope, hostname):
+        return '250 alex.example.com'
+
+
+class MAILHandler:
+    @asyncio.coroutine
+    def handle_MAIL(self, server, session, envelope, address, options):
+        envelope.mail_options.extend(options)
+        return '250 Yeah, sure'
+
+
+class RCPTHandler:
+    @asyncio.coroutine
+    def handle_RCPT(self, server, session, envelope, address, options):
+        envelope.rcpt_options.extend(options)
+        if address == 'bart@example.com':
+            return '550 Rejected'
+        envelope.rcpt_tos.append(address)
+        return '250 OK'
+
+
+class DATAHandler:
+    @asyncio.coroutine
+    def handle_DATA(self, server, session, envelope):
+        return '599 Not today'
+
+
+class NoHooksHandler:
+    pass
+
+
+class TestHooks(unittest.TestCase):
+    def test_rcpt_hook(self):
+        controller = Controller(RCPTHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            with self.assertRaises(SMTPRecipientsRefused) as cm:
+                client.sendmail('anne@example.com', ['bart@example.com'], """\
+From: anne@example.com
+To: bart@example.com
+Subject: Test
+
+""")
+            self.assertEqual(cm.exception.recipients, {
+                'bart@example.com': (550, b'Rejected'),
+                })
+
+    def test_helo_hook(self):
+        controller = Controller(HELOHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            code, response = client.helo('me')
+        self.assertEqual(code, 250)
+        self.assertEqual(response, b'geddy.example.com')
+
+    def test_ehlo_hook(self):
+        controller = Controller(EHLOHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            code, response = client.ehlo('me')
+        self.assertEqual(code, 250)
+        lines = response.decode('utf-8').splitlines()
+        self.assertEqual(lines[-1], 'alex.example.com')
+
+    def test_mail_hook(self):
+        controller = Controller(MAILHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.helo('me')
+            code, response = client.mail('anne@example.com')
+        self.assertEqual(code, 250)
+        self.assertEqual(response, b'Yeah, sure')
+
+    def test_data_hook(self):
+        controller = Controller(DATAHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            with self.assertRaises(SMTPDataError) as cm:
+                client.sendmail('anne@example.com', ['bart@example.com'], """\
+From: anne@example.com
+To: bart@example.com
+Subject: Test
+
+Yikes
+""")
+            self.assertEqual(cm.exception.smtp_code, 599)
+            self.assertEqual(cm.exception.smtp_error, b'Not today')
+
+    def test_no_hooks(self):
+        controller = Controller(NoHooksHandler())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.helo('me')
+            client.mail('anne@example.com')
+            client.rcpt(['bart@example.com'])
+            code, response = client.data("""\
+From: anne@example.com
+To: bart@example.com
+Subject: Test
+
+""")
+            self.assertEqual(code, 250)
+
+
 class CapturingServer(Server):
     def __init__(self, *args, **kws):
         self.warnings = None
@@ -510,6 +622,38 @@ class AsyncDeprecatedHandler:
         pass
 
 
+class DeprecatedHookServer(Server):
+    def __init__(self, *args, **kws):
+        self.warnings = None
+        super().__init__(*args, **kws)
+
+    @asyncio.coroutine
+    def smtp_EHLO(self, arg):
+        with patch('aiosmtpd.smtp.warn') as mock:
+            yield from super().smtp_EHLO(arg)
+        self.warnings = mock.call_args_list
+
+    @asyncio.coroutine
+    def smtp_RSET(self, arg):
+        with patch('aiosmtpd.smtp.warn') as mock:
+            yield from super().smtp_RSET(arg)
+        self.warnings = mock.call_args_list
+
+    @asyncio.coroutine
+    def ehlo_hook(self):
+        pass
+
+    @asyncio.coroutine
+    def rset_hook(self):
+        pass
+
+
+class DeprecatedHookController(Controller):
+    def factory(self):
+        self.smtpd = DeprecatedHookServer(self.handler)
+        return self.smtpd
+
+
 class TestDeprecation(unittest.TestCase):
     # handler.process_message() is deprecated.
     def test_deprecation(self):
@@ -527,7 +671,7 @@ Testing
         self.assertEqual(len(controller.smtpd.warnings), 1)
         self.assertEqual(
             controller.smtpd.warnings[0],
-            call('Use handler.handle_DATA instead of .process_message()',
+            call('Use handler.handle_DATA() instead of .process_message()',
                  DeprecationWarning))
 
     def test_deprecation_async(self):
@@ -545,5 +689,29 @@ Testing
         self.assertEqual(len(controller.smtpd.warnings), 1)
         self.assertEqual(
             controller.smtpd.warnings[0],
-            call('Use handler.handle_DATA instead of .process_message()',
+            call('Use handler.handle_DATA() instead of .process_message()',
+                 DeprecationWarning))
+
+    def test_ehlo_hook_deprecation(self):
+        controller = DeprecatedHookController(Sink())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.ehlo('example.com')
+        self.assertEqual(len(controller.smtpd.warnings), 1)
+        self.assertEqual(
+            controller.smtpd.warnings[0],
+            call('Use handler.handle_EHLO() instead of .ehlo_hook()',
+                 DeprecationWarning))
+
+    def test_rset_hook_deprecation(self):
+        controller = DeprecatedHookController(Sink())
+        controller.start()
+        self.addCleanup(controller.stop)
+        with SMTP(controller.hostname, controller.port) as client:
+            client.rset()
+        self.assertEqual(len(controller.smtpd.warnings), 1)
+        self.assertEqual(
+            controller.smtpd.warnings[0],
+            call('Use handler.handle_RSET() instead of .rset_hook()',
                  DeprecationWarning))
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index fa565fb..b3e8dae 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -32,8 +32,9 @@ class ReceivingHandler:
         self.box = []
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         self.box.append(envelope.content)
+        return '250 OK'
 
 
 class SizedController(Controller):
@@ -66,7 +67,7 @@ class ErroringHandler:
     error = None
 
     @asyncio.coroutine
-    def handle_DATA(self, session, envelope):
+    def handle_DATA(self, server, session, envelope):
         return '499 Could not accept the message'
 
     @asyncio.coroutine
@@ -681,9 +682,9 @@ Subject: A test
 
 Testing
 """)
-                self.assertEqual(cm.exception.code, 499)
-                self.assertEqual(cm.exception.response,
-                                 b'Could not accept the message')
+            self.assertEqual(cm.exception.smtp_code, 499)
+            self.assertEqual(cm.exception.smtp_error,
+                             b'Could not accept the message')
 
     def test_too_long_message_body(self):
         controller = SizedController(Sink(), size=100)
@@ -748,7 +749,7 @@ Testing
 
 class TestCustomizations(unittest.TestCase):
     def test_custom_hostname(self):
-        controller = CustomHostnameController(Sink)
+        controller = CustomHostnameController(Sink())
         controller.start()
         self.addCleanup(controller.stop)
         with SMTP(controller.hostname, controller.port) as client:
@@ -757,7 +758,7 @@ class TestCustomizations(unittest.TestCase):
             self.assertEqual(response, bytes('custom.localhost', 'utf-8'))
 
     def test_custom_greeting(self):
-        controller = CustomIdentController(Sink)
+        controller = CustomIdentController(Sink())
         controller.start()
         self.addCleanup(controller.stop)
         with SMTP() as client:
@@ -767,7 +768,7 @@ class TestCustomizations(unittest.TestCase):
             self.assertEqual(msg[-22:], b'Identifying SMTP v2112')
 
     def test_default_greeting(self):
-        controller = Controller(Sink)
+        controller = Controller(Sink())
         controller.start()
         self.addCleanup(controller.stop)
         with SMTP() as client:
@@ -777,7 +778,7 @@ class TestCustomizations(unittest.TestCase):
             self.assertEqual(msg[-len(GREETING):], bytes(GREETING, 'utf-8'))
 
     def test_mail_invalid_body_param(self):
-        controller = NoDecodeController(Sink)
+        controller = NoDecodeController(Sink())
         controller.start()
         self.addCleanup(controller.stop)
         with SMTP() as client: