classification
Title: smtpd cannot be used without affecting global state
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vinay.sajip Nosy List: barry, giampaolo.rodola, maker, pitrou, python-dev, r.david.murray, terry.reedy, vinay.sajip
Priority: normal Keywords: patch

Created on 2011-04-29 21:12 by vinay.sajip, last changed 2013-06-07 14:21 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
proposed-changes.diff vinay.sajip, 2012-06-04 08:43 Changes to asyncore, smtpd and test_logging review
revised-changes.diff vinay.sajip, 2013-06-01 20:11 Changes to smtpd and test_logging review
incorporate-feedback.diff vinay.sajip, 2013-06-06 09:00 Addressed Giampaolo's comments review
Repositories containing patches
http://hg.python.org/sandbox/vsajip#fix11959
Messages (29)
msg134812 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-04-29 21:12
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.
msg134826 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-04-30 01:03
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?
msg134866 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-04-30 15:24
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.
msg134910 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-05-01 14:00
I've made a patch. See

https://bitbucket.org/vinay.sajip/cpython-smtpd/compare/default..mirror/cpython
msg135373 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-05-06 21:57
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.
msg135378 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-05-06 22:47
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.
msg135390 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-05-07 00:11
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.
msg135709 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2011-05-10 15:11
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.
msg155860 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-15 05:43
The test failure in #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?
msg155881 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-03-15 12:14
> 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.
msg160675 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-15 01:17
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?
msg160727 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-05-15 13:39
> 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.
msg160728 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2012-05-15 13:48
Changing it in asyncore is fine.
msg160733 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-05-15 14:51
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.
msg160741 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-05-15 16:25
> 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).
msg160752 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-15 19:29
> 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.
msg160763 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-05-15 21:15
> 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?
msg160765 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-15 21:24
> 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.
msg160814 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-05-16 09:14
> 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.
msg160815 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-16 09:18
> 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)
msg160817 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2012-05-16 09:25
>
> 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.
msg189776 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-05-21 18:27
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.
msg189778 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2013-05-21 19:07
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.
msg190456 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2013-06-01 20:20
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.
msg190617 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-06-04 21:55
The changes to smtpd.py seem reasonable to me.  I see no reason not to make this change, so +1.
msg190624 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-06-04 22:32
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)
msg190708 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-06-06 09:13
LGTM now.
msg190712 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-06-06 11:03
Looks good to me too.
msg190747 - (view) Author: Roundup Robot (python-dev) Date: 2013-06-07 14:21
New changeset ed498f477549 by Vinay Sajip in branch 'default':
Closes #11959: SMTPServer and SMTPChannel now take an optional map, use of which avoids affecting global state.
http://hg.python.org/cpython/rev/ed498f477549
History
Date User Action Args
2013-06-07 14:21:55python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg190747

resolution: fixed
stage: patch review -> resolved
2013-06-06 11:03:37r.david.murraysetmessages: + msg190712
2013-06-06 09:13:51giampaolo.rodolasetmessages: + msg190708
2013-06-06 09:00:40vinay.sajipsetfiles: + incorporate-feedback.diff
2013-06-04 22:32:17giampaolo.rodolasetmessages: + msg190624
2013-06-04 21:55:11barrysetmessages: + msg190617
2013-06-01 20:20:32vinay.sajipsetmessages: + msg190456
2013-06-01 20:12:08vinay.sajipsetfiles: + revised-changes.diff
2013-05-21 19:07:24vinay.sajipsetmessages: + msg189778
2013-05-21 18:27:47giampaolo.rodolasetmessages: + msg189776
2013-05-21 18:03:31barrysetnosy: + barry
2012-06-28 07:45:57vinay.sajipsetversions: + Python 3.4, - Python 3.3
2012-06-04 08:54:05vinay.sajipsetassignee: vinay.sajip
stage: patch review
2012-06-04 08:43:42vinay.sajipsetfiles: + proposed-changes.diff
keywords: + patch
2012-06-04 08:42:56vinay.sajipsethgrepos: + hgrepo129
2012-05-26 15:22:36r.david.murrayunlinkissue8739 dependencies
2012-05-16 09:25:21vinay.sajipsetmessages: + msg160817
2012-05-16 09:18:58pitrousetmessages: + msg160815
2012-05-16 09:14:57vinay.sajipsetmessages: + msg160814
2012-05-15 21:24:50pitrousetmessages: + msg160765
2012-05-15 21:15:13vinay.sajipsetmessages: + msg160763
2012-05-15 19:29:05pitrousetnosy: + pitrou
messages: + msg160752
2012-05-15 16:25:30vinay.sajipsetmessages: + msg160741
2012-05-15 14:51:56r.david.murraysetmessages: + msg160733
2012-05-15 13:48:02giampaolo.rodolasetmessages: + msg160728
2012-05-15 13:39:20vinay.sajipsetmessages: + msg160727
2012-05-15 01:17:50r.david.murraysetmessages: + msg160675
2012-03-17 16:53:53makersetnosy: + maker
2012-03-15 12:14:53vinay.sajipsetmessages: + msg155881
2012-03-15 05:49:32r.david.murraylinkissue8739 dependencies
2012-03-15 05:43:07r.david.murraysetmessages: + msg155860
2011-05-10 15:11:11vinay.sajipsetmessages: + msg135709
2011-05-07 00:11:30terry.reedysetmessages: + msg135390
2011-05-06 22:47:40vinay.sajipsetmessages: + msg135378
2011-05-06 21:57:53terry.reedysetnosy: + terry.reedy
messages: + msg135373
2011-05-01 14:00:59vinay.sajipsetmessages: + msg134910
2011-04-30 15:24:06vinay.sajipsetmessages: + msg134866
2011-04-30 01:03:58giampaolo.rodolasetmessages: + msg134826
2011-04-30 01:03:14giampaolo.rodolasetmessages: - msg134825
2011-04-30 01:02:51giampaolo.rodolasetmessages: + msg134825
2011-04-29 21:50:29r.david.murraysetnosy: + r.david.murray
2011-04-29 21:12:29vinay.sajipcreate