smtpd cannot be used without affecting global state

[vsajip]

BPO	11959
Nosy	@warsaw, @terryjreedy, @vsajip, @pitrou, @giampaolo, @bitdancer, @mmaker
Files	proposed-changes.diff: Changes to asyncore, smtpd and test_logging revised-changes.diff: Changes to smtpd and test_logging incorporate-feedback.diff: Addressed Giampaolo's comments

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/vsajip'
closed_at = <Date 2013-06-07.14:21:55.434>
created_at = <Date 2011-04-29.21:12:29.739>
labels = ['type-bug', 'library']
title = 'smtpd cannot be used without affecting global state'
updated_at = <Date 2013-06-07.14:21:55.427>
user = 'https://github.com/vsajip'

bugs.python.org fields:

activity = <Date 2013-06-07.14:21:55.427>
actor = 'python-dev'
assignee = 'vinay.sajip'
closed = True
closed_date = <Date 2013-06-07.14:21:55.434>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2011-04-29.21:12:29.739>
creator = 'vinay.sajip'
dependencies = []
files = ['25815', '30441', '30480']
hgrepos = ['129']
issue_num = 11959
keywords = ['patch']
message_count = 29.0
messages = ['134812', '134826', '134866', '134910', '135373', '135378', '135390', '135709', '155860', '155881', '160675', '160727', '160728', '160733', '160741', '160752', '160763', '160765', '160814', '160815', '160817', '189776', '189778', '190456', '190617', '190624', '190708', '190712', '190747']
nosy_count = 8.0
nosy_names = ['barry', 'terry.reedy', 'vinay.sajip', 'pitrou', 'giampaolo.rodola', 'r.david.murray', 'maker', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11959'
versions = ['Python 3.4']

[vsajip]

It seems not possible to use smtpd in certain contexts, because it forces use of global state. For example, I'm looking at implementing a test SMTP server to test logging's SMTPHandler. Neither SMTPServer nor SMTPChannel allow a map to be passed in, forcing use of the global socket_map in asyncore. Both of these classes should accept an optional map argument - the map passed to SMTPServer should be stored in the server instance and passed when creating an SMTPChannel in handle_accepted.

[giampaolo]

The fact that you need to keep two separate maps makes me think that the approach you have in mind might be wrong, as in - that's something you usually don't want to do -. The fact that asyncore uses a global socket map is surely unfortunate, but it's something you rarely want to deal with, therefore changing the __init__ notation of those classes is debatable at least.
What are you trying to achieve exactly?

[vsajip]

I don't want to use two different maps - I just want to use a single map which is not the global "socket_map" in asyncore.

asyncore.dispatcher and asynchat.async_chat allow for a map to be passed in so that the default global is not used, but smtpd does not allow this. Note that asyncore.loop() also allows a map to be passed in, so I'm sure this functionality is by design.

I mentioned two places where the map is to be used - passed to SMTPServer constructor (and saved in SMPTServer instance) and the *same* map used to initialise the SMTPChannel from SMTPServer.handle_accepted().

Since asyncore and asynchat support using a passed-in map to avoid using a global, it's not unreasonable to expect smtpd to support it too. After all, using it relies on asyncore.loop(), and passing an explicit map is allowed here.

I initially came across this because I got some warnings from regrtest.py about changed state, when I was trying to implement a TestSMTPServer class (derived from smtpd.SMTPServer) to test the SMTPHandler in logging.

I've taken out the functionality from test_logging for now, but I have a test script here:

https://gist.github.com/949744

This successfully uses a non-global map ("my_map"), but notice how much I had to resort to copypasta.

If I've missed some neat solution which avoids this hackery, please let me know! This is my first use of the smtpd module :-)

As I say, what I'm trying to do is to avoid changing global state in the unit test suite.

[vsajip]

I've made a patch. See

https://bitbucket.org/vinay.sajip/cpython-smtpd/compare/default..mirror/cpython

[terryjreedy]

I looked as the small patch to smptd.py. This strikes me a a reasonable use of dependency injection to make smptd more usable in testing, especially given that asyncx are have it.

The only thing I do not understand fully is the redefinition of set_socket, perhaps because I do not know where the original is that is being replaced.

[vsajip]

It's create_socket which is being redefined, because the base class implementation

https://bitbucket.org/mirror/cpython/src/5661480f7763/Lib/asyncore.py#cl-292

calls set_socket without passing a map.

[terryjreedy]

OK, passing self.sockmap=None to setsocket matches its current behavior, so your new code should not change any current smtpd users, while modifying asyncore.dispatcher.create_socket might possibly affect someone.

[vsajip]

The overridden create_socket() method will have the same behaviour for the case when a socket map is *not* passed in to smtpd.__init__(). Users using the existing signature for the constructor will cause the sockmap instance attribute to be set to None, and this will get passed in to set_socket just as was happening before. So the only time the overridden create_socket will behave differently is if a non-None value is passed into smtpd.__init__(), and that's by design.

Of course there is a slightly increased maintenance burden, in that other functional changes to asyncore.dispatcher.create_socket will need to be duplicated in smtpd.create_socket. However, such changes would be fairly infrequent, methinks. A comment could be added to asyncore.dispatcher.create_socket if necessary, to remind maintainers about this.

[bitdancer]

The test failure in bpo-14314 isn't a bug in my code, it is due to the fact that you copied the __init__ method of SMTPChannel in your logging tests, and the __init__ is changed by my patch. Clearly it would be good to resolve this issue one way or another.

I notice that your logging test code doesn't override create_socket. Is that no longer needed?

[vsajip]

I notice that your logging test code doesn't override create_socket. Is that no longer needed?

I removed that code from test_logging, but the code in the BitBucket repo shows how I'd like smtpd to be changed.

I would of course prefer to avoid copying stuff from SMTPChannel, as the docstring for TestSMTPChannel indicates. If you could take a look at the suggested changes in

https://bitbucket.org/vinay.sajip/cpython-smtpd/compare/default..mirror/cpython

and let me have your comments, that'd be great. The code in that repo does redefine create_socket.

[bitdancer]

I'm finally getting back around to this.

If asyncore and asynchat are (mostly?) supporting an alternate socket map, why is it necessary to copy create_socket? Shouldn't we be fixing create_socket in asyncore instead?

[vsajip]

If asyncore and asynchat are (mostly?) supporting an alternate
socket map, why is it necessary to copy create_socket?
Shouldn't we be fixing create_socket in asyncore instead?

Well, I don't see how this can be done along with keeping existing behaviour, since if you currently pass a map to the dispatcher constructor, it's not passed to set_socket; fixing it in asyncore would mean changing this fact, and so in theory could break existing code.

[giampaolo]

Changing it in asyncore is fine.

[bitdancer]

But it is create_socket you want to change. So if we add a map argument to that and only pass it to socket if it is non-None, wouldn't that maintain backward compatibility with current asyncore behavior? Neither asyncore nor asynchat calls create_socket, as far as I can see.

[vsajip]

So if we add a map argument to that and only pass it to socket if it
is non-None, wouldn't that maintain backward compatibility with
current asyncore behavior?

Sorry I was being a bit dense ... it's been a while since I looked at this. I think you are right that the base create_socket could be changed in this way. I'll work up a patch in my sandbox branch (for easier Rietveld integration).

[pitrou]

I initially came across this because I got some warnings from
regrtest.py about changed state, when I was trying to implement a
TestSMTPServer class (derived from smtpd.SMTPServer) to test the
SMTPHandler in logging.

If you get a warning, it means your tests lack proper cleanup, so you should fix that instead of trying to make the warning disappear by circumventing regrtest's detection mechanism.

[vsajip]

If you get a warning, it means your tests lack proper cleanup, so you
should fix that instead of trying to make the warning disappear by
circumventing regrtest's detection mechanism.

What makes you say I was trying to circumvent regrtest's detection mechanism? I wasn't. Isn't it the case that tests shouldn't affect global state? Since regrtest told me that global state was being changed by the smtpd module used by the test, I tried to find a way of avoiding changing global state in my test - but because of the problem I mention, I couldn't see a way of using smtpd without affecting global state. This is partly because of an underlying wart in asyncore, which this issue is trying to address.

Do you have a proposal about how to solve this - is there something you think I've missed? Do you have specific concerns about the approach being discussed?

[pitrou]

I tried to find a way of avoiding changing global state in my test -
but because of the problem I mention, I couldn't see a way of using
smtpd without affecting global state.

Well, other tests manage it even without using a private socket map.
Leaving dangling sockets in the socket map could mean your code forgets
to close them, for example. So it's better to fix the code or the tests,
rather than try to defeat the detection mechanism.

[vsajip]

Well, other tests manage it even without using a private socket map.
Leaving dangling sockets in the socket map could mean your code
forgets to close them, for example.

This issue is not about getting test_logging to work in a particular way; test_logging is exercising SMTPHandler and (AFAIK) tidying up after itself, with no sockets left open.

When working on the test, I just noticed that smtpd forces use of the global socket map, which is not ideal ("The fact that asyncore uses a global socket map is surely unfortunate" - Giampaolo). Given that asyncore's design allows for a socket map to be passed in (at least in part - RDM's comment), ISTM that it should support this consistently, and also that smtpd should support this mode of use.

[pitrou]

Given that asyncore's design allows for a socket map to be passed in
(at least in part - RDM's comment), ISTM that it should support this
consistently, and also that smtpd should support this mode of use.

Well, I would argue that asyncore's design is thoroughly broken, and
passing a socket map is a poor kludge to avoid global state; in a
sophisticated event loop, the socket map wouldn't be the only piece of
state to pass around.
(look at twisted's reactors for a comparison)

[vsajip]

Well, I would argue that asyncore's design is thoroughly broken, and

passing a socket map is a poor kludge to avoid global state; in a
sophisticated event loop, the socket map wouldn't be the only piece of
state to pass around.

I don't disagree with you, but since it's there in the stdlib, there's no reason not to make incremental improvements involving small changes.

[giampaolo]

Looking back at this I think that allowing a map argument to be passed to SMTPChannel in order to allow running handlers in separate threads can be reasonable after all.
I don't understand why create_socket() signature needs to be changed though.

[vsajip]

I wasn't suggesting changing the signature of create_socket, I just thought that it needed to be reimplemented because it didn't pass a map to set_socket. I've had a look at it again, and a reimplementation of create_socket doesn't seem to be needed, after all, because:

SMTPServer.__init__ can pass the map it received to dispatcher.__init__, which will cause it to be set in self._map. Then, when create_socket calls set_socket with no map, that will call add_channel with map=None, which will then cause self._map to be used.

I'll update the patch as soon as I get a chance.

[vsajip]

Patch now updated to revert asyncore changes. The changes are now:

smtpd.py - changed SMTPChannel and SMTPServer to accept map argument

test_logging.py - removed subclassed SMTPChannel, not needed since the base SMTPChannel class now accepts a map, and simplified TestSMTPServer, since the base SMTPServer class now accepts a map.

[warsaw]

The changes to smtpd.py seem reasonable to me. I see no reason not to make this change, so +1.

[giampaolo]

Changes to Doc/library/asyncore.rst should be reverted.

Also I would do:

• asynchat.async_chat.__init__(self, conn, map)
  + asynchat.async_chat.__init__(self, conn, map=map)

[giampaolo]

LGTM now.

[bitdancer]

Looks good to me too.

[python-dev]

New changeset ed498f477549 by Vinay Sajip in branch 'default':
Closes bpo-11959: SMTPServer and SMTPChannel now take an optional map, use of which avoids affecting global state.
http://hg.python.org/cpython/rev/ed498f477549