0
\$\begingroup\$

My code right now look like this

@contextmanager
def smtp_connection(host, user=None, passwd=None, timeout=5):
    conn = None # smell here
    try:
        conn = SMTP(host=host, timeout=timeout)
        conn.ehlo_or_helo_if_needed()
        if user and passwd:
            conn.login(user=user, password=passwd)
        logger.debug('SMTP connected')
        yield conn
    except Exception as e:
        raise e
    finally:
        if conn: # and here
            conn.quit()

In recipes for ExitStack there is a suggestion to replace try-finally and flag variables with this

with ExitStack() as stack:
    stack.callback(cleanup_resources)
    result = perform_operation()
    if result:
        stack.pop_all()

But this doesn't use result in cleanup_resources. So in my case it still would be

with ExitStack() as stack:
    result = None
    stack.callback(lambda conn: conn.quit())
    result = POP3() # code from above here
    if result:
        stack.pop_all()
\$\endgroup\$
5
  • 2
    \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does \$\endgroup\$ – 301_Moved_Permanently Apr 20 '16 at 9:49
  • \$\begingroup\$ IIRC they added __enter__ and __exit__ for this pattern. \$\endgroup\$ – Peilonrayz Apr 20 '16 at 9:55
  • \$\begingroup\$ @MathiasEttinger I think the code is self explanatory. It's context manager that return SMTP connection. Isn't that obvious? \$\endgroup\$ – user1685095 Apr 20 '16 at 10:10
  • \$\begingroup\$ @JoeWallis that's what contextmanager decorator is for. To simplify enter and exit stuff. \$\endgroup\$ – user1685095 Apr 20 '16 at 10:11
  • 1
    \$\begingroup\$ @user1685095 Yes, obviously it is a context manager returning an SMTP connection. But why do you need it? What are your needs that lead you to that? How do you use it? I also don't really understand the relation between your code and the two snippets below it (except the fact that they both use contextlib resources). The more context you provide, the more interesting answers you’ll be able to get. (and optionally the more users will be insterested into giving such an answer.) \$\endgroup\$ – 301_Moved_Permanently Apr 20 '16 at 12:27
2
\$\begingroup\$
except Exception as e:
    raise e

First, you could simplify this to

except Exception:
    raise

Second, since you only re-raise it, you are not prepared to handle any Exception that could appear either with your code or the code managing the connection returned by your context manager. Thus you don't need that except clause.

Now the only thing left to manage is the state of the connection. Since there is no exception handling performed by your code, you are free to create the connection out of the try … finally and use that mechanism to only close the connection whatever happened:

@contextmanager
def smtp_connection(host, user=None, passwd=None, timeout=5):
    conn = SMTP(host=host, timeout=timeout)
    try:
        conn.ehlo_or_helo_if_needed()
        if user and passwd:
            conn.login(user=user, password=passwd)
        logger.debug('SMTP connected')
        yield conn
    finally:
        conn.quit()

You could also simplify the design by separating concerns. As it stand, your function does two things:

  • manage the connection;
  • perform some initial setup.

By delegating to a second context manager, you could separate these two behaviors into reusable bits:

@contextmanager
def smtp_connection(host, timeout):
    connection = SMTP(host=host, timeout=timeout)
    try:
        yield connection
    finally:
        connection.quit()

@contextmanager
def smtp_setup(host, user=None, passwd=None, timeout=5):
    with smtp_connection(host, timeout) as conn:
        conn.ehlo_or_helo_if_needed()
        if user and passwd:
            conn.login(user=user, password=passwd)
        logger.debug('SMTP connected')
        yield conn

But, looking at the second context manager, there is nothing to manage anymore since there is no teardown/cleanup anymore. Thus it is best to provide it as an independant function:

@contextmanager
def smtp_connection(host, timeout=5):
    connection = SMTP(host=host, timeout=timeout)
    try:
        yield connection
    finally:
        connection.quit()

def smtp_setup(conn, user=None, passwd=None):
    conn.ehlo_or_helo_if_needed()
    if user and passwd:
        conn.login(user=user, password=passwd)
    logger.debug('SMTP connected')

You would then need to change your calling code from

with smtp_connection(h, u, p, t) as conn:
    # do stuff

to

with smtp_connection(h, t) as conn:
    smtp_setup(conn, u, p)
    # do stuff

In the end, I personally don't like to have to manage the try ... finally inside the context manager. I don't find that natural as I preffer to write an explicit class using __enter__ and __exit__:

class smtp_connection:
    def __init__(self, host, timeout):
        self.smtp = SMTP(host=host, timeout=timeout)
    def __enter__(self):
        return self.smtp
    def __exit__(self, exc_type, exc_value, exc_trace):
        self.smtp.quit()

Using a class could also let you wrap this thing around SMTP directly:

class smtp_connection(SMTP):
    def __init__(self, host, timeout):
        super().__init__(host=host, timeout=timeout)
    def __enter__(self):
        return self
    def __exit__(self, exc_type, exc_value, exc_trace):
        self.quit()
    def setup(self, user=None, password=None):
        self.ehlo_or_helo_if_needed()
        if user and passwd:
            self.login(user=user, password=passwd)
        logger.debug('SMTP connected')

Use it like:

with smtp_connection(h, t) as conn:
    conn.setup(u, p)
    # do stuff
\$\endgroup\$
6
  • \$\begingroup\$ You're missing a try/finally in smtp_connection. This way quit won't be called on exceptions. \$\endgroup\$ – Sjoerd Job Postmus Apr 21 '16 at 5:07
  • \$\begingroup\$ @SjoerdJobPostmus Right, I'm not quite using the decorator so I got the behavior somewhat mixed up. \$\endgroup\$ – 301_Moved_Permanently Apr 21 '16 at 6:53
  • \$\begingroup\$ @MathiasEttinger I agree on first point about reraising. Separation of concerns isn't a concern here (pun intended) because the connection without login will be never needed, but that's just details. What I'm dubious about is removing conn = SMTP(...) from try except block. If we got for example connection refused exception the finally block wouldn't be called. And shouldn't connection have some resources in that case (like socket that it probably opened to trying to connect etc)? \$\endgroup\$ – user1685095 Apr 21 '16 at 7:26
  • \$\begingroup\$ @MathiasEttinger Judging by the code of stmplib it's wouldn't close the socket in case of errors. So AFAIK you code has resource leak. \$\endgroup\$ – user1685095 Apr 21 '16 at 8:10
  • 2
    \$\begingroup\$ @user1685095 And what do you think happens in your code in the exact same case? SMTP(...) raise an exception so the assignement conn = .. doesn't occur. Thus, in the finally block, the if conn: doesn't trigger either. Both code handle that the same way. \$\endgroup\$ – 301_Moved_Permanently Apr 21 '16 at 8:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.