[issue11959] smtpd cannot be used without affecting global state

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev
resolution:  -> fixed
stage: patch review -> committed/rejected
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[R. David Murray]

R. David Murray added the comment:

Looks good to me too.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

LGTM now.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


Added file: http://bugs.python.org/file30480/34974b9bc4d0.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Barry A. Warsaw]

Barry A. Warsaw added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


Added file: http://bugs.python.org/file30441/d7c50c15468d.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Barry A. Warsaw]

Changes by Barry A. Warsaw :


--
nosy: +barry

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


--
versions: +Python 3.4 -Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


--
assignee:  -> vinay.sajip
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


--
keywords: +patch
Added file: http://bugs.python.org/file25815/fa55dc894947.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Changes by Vinay Sajip :


--
hgrepos: +129

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

>
> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Antoine Pitrou]

Antoine Pitrou  added the comment:

> 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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Antoine Pitrou]

Antoine Pitrou  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

> 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Antoine Pitrou]

Antoine Pitrou  added the comment:

> 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.

--
nosy: +pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

> 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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[R. David Murray]

R. David Murray  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

Changing it in asyncore is fine.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[R. David Murray]

R. David Murray  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Michele Orrù]

Changes by Michele Orrù :


--
nosy: +maker

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[R. David Murray]

R. David Murray  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Terry J. Reedy]

Terry J. Reedy  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Terry J. Reedy]

Terry J. Reedy  added the comment:

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.

--
nosy: +terry.reedy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

I've made a patch. See

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

Vinay Sajip  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Changes by Giampaolo Rodola' :


--
Removed message: http://bugs.python.org/msg134825

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Giampaolo Rodola']

Giampaolo Rodola'  added the comment:

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 default 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[R. David Murray]

Changes by R. David Murray :


--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11959] smtpd cannot be used without affecting global state

[Vinay Sajip]

New submission from Vinay Sajip :

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.

--
components: Library (Lib)
messages: 134812
nosy: giampaolo.rodola, vinay.sajip
priority: normal
severity: normal
status: open
title: smtpd cannot be used without affecting global state
type: behavior
versions: Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com