classification
Title: smtpd.PureProxy and smtpd.MailmanProxy broken by extra kwargs, bytes and more
Type: behavior Stage: resolved
Components: email Versions: Python 3.8, Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: barry, eamanu, r.david.murray, samuelcolvin
Priority: normal Keywords: patch, patch, patch, patch

Created on 2019-01-20 11:14 by samuelcolvin, last changed 2019-01-21 17:28 by samuelcolvin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11624 closed samuelcolvin, 2019-01-20 12:25
PR 11624 closed samuelcolvin, 2019-01-20 12:25
PR 11624 closed samuelcolvin, 2019-01-20 12:25
PR 11624 closed samuelcolvin, 2019-01-20 12:25
Messages (10)
msg334083 - (view) Author: Samuel Colvin (samuelcolvin) * Date: 2019-01-20 11:14
smtpd.PureProxy.process_message and smtpd.MailmanProxy.process_message are defined to not receive the extra kwargs which they're called with.

They both also expect "data" to be str when it's actually bytes.

Thus they're completed broken at the moment.

I'd like to submit a PR to fix these two bugs.

There are a number of other issues/potential improvements to smtpd which are not critical but I guess should be fixed:
* no support for starttls
* use of print(..., file=DEBUGSTREAM) instead of logger.debug
* no type hints
* PureProxy's forwarding doesn't try starttls

Should I create a new issue(s) for these problems or is there some agreement that only actual bugs will be fixed in little-used modules like this?
msg334086 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-20 15:08
> Should I create a new issue(s) for these problems or is there some agreement that only actual bugs will be fixed in little-used modules like this?

I would create new issue for each point to discuss, but I would like listen other opinions
msg334088 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2019-01-20 15:12
Thanks for being willing to work on this, but smtpd is deprecated in favor of aiosmtpd (which is not part of the stdlib).  smtpd should really only be used for internal stdlib testing, but it is retained for backward compatibility reasons.

All of these issues have been previously reported and were closed in favor of recommending aiosmtpd because we are no longer maintaining smtpd.
msg334090 - (view) Author: Samuel Colvin (samuelcolvin) * Date: 2019-01-20 15:23
Thanks for the explanation David.

It would be really useful if this was prominently noted in smtpd, it would have saved me spending my morning fixing these things. There's also (AFAIK) nothing about this being deprecated in the docs.

More generally I understand the idea of deprecated code that doesn't get updated to conform to latest conventions, but surely if the code exists in the standard lib. it should at least work as it was originally intended or be removed?

Since the PR is done and passing surely it would be better to merge it than leave PureProxy completely broken.

By the way, aiosmtpd is not in good shape:
1. Its proxy handler is blocking https://github.com/aio-libs/aiosmtpd/blob/master/aiosmtpd/handlers.py#L119
2. Its proxy handler appears to be broken in some other way, I couldn't get it to forward emails which is why I resorted to using standard smtpd.
3. It's built in a very odd way, it appears to use threading not asyncio to run the main controller https://github.com/aio-libs/aiosmtpd/blob/master/aiosmtpd/controller.py#L54
msg334148 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2019-01-21 16:39
It is documented as deprecated, but only in the 'seealso' note at the top.  I think it would be reasonable to open an issue to add an actual 'deprecated' ReST tag to the docs.

For your 1 and 2, the stdlib smtpd forwarding is also blocking; that code appears to be copied verbatim from stdlib smtpd.  It needs to be replaced with calls to aiosmtplib.  Feel free to submit a PR :)

For your 3, that design is so that you can run the smtpd server as part of a non-asyncio application (eg: for testing, which has always been the main purpose of this module).  For an asyncio-only application you would not use a Controller (unless you wanted to offload the smtp traffic to a different thread, of course, then it would be useful :)
msg334149 - (view) Author: Samuel Colvin (samuelcolvin) * Date: 2019-01-21 16:46
Thanks for the response.

I've created issues on aiosmtpd for both these things. There are much better ways of running the controller than threading, but that's a discussion for https://github.com/aio-libs/aiosmtpd/issues/160. I'll try and work on aiosmtpd in the future.

Regarding smtpd, please could you respond to:

> More generally I understand the idea of deprecated code that doesn't get updated to conform to latest conventions, but surely if the code exists in the standard lib. it should at least work as it was originally intended or be removed?

Of course, I know part of this is my wish to get my PR merged into cpython, but I really don't see any argument for leaving completely broken code in the standard library.

Surely either it should be removed before the next minor release or fixed?

I would say PureProxy could still be useful and should be left in place while MailmanProxy should be removed.
msg334152 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2019-01-21 17:06
The mailman proxy has been abandoned for a long time now, so no fixes there.  I have some sympathy to fixing PureProxy, but since the stdlib itself doesn't use it, not a lot :)

At some point we will start cleaning up old code (probably a while after 2.7 is dead) after going through a release-deprecation-cycle, but that time hasn't arrived yet.  smtpd itself will stay because the stdlib test suite uses it, but you are right that MailmanProxy can be removed, as well as PrueProxy if we don't fix it.

We are not in general going to fix bugs in it that don't impact the stdlib itself, but if Barry (or someone else) wants to review and accept the PureProxy PR that's fine with me.  I don't myself have time to review it, I'm afraid.
msg334153 - (view) Author: Samuel Colvin (samuelcolvin) * Date: 2019-01-21 17:08
Ok. Thanks for your explanation. Makes sense.
msg334155 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2019-01-21 17:10
Actually, thinking about it some more, you are right.  If PureProxy doesn't at least function according to the current docs it should either be fixed or we should deprecate-and-remove it in 3.8 or 3.9 (depending on how strongly people feel about the deprecation cycle), and we should remove MailmanProxy.  Feel free to open new, targeted issues for these.
msg334158 - (view) Author: Samuel Colvin (samuelcolvin) * Date: 2019-01-21 17:28
Great, https://bugs.python.org/issue35799 and https://bugs.python.org/issue35800 created.
History
Date User Action Args
2019-01-21 17:28:18samuelcolvinsetmessages: + msg334158
2019-01-21 17:10:54r.david.murraysetkeywords: patch, patch, patch, patch

messages: + msg334155
2019-01-21 17:08:24samuelcolvinsetmessages: + msg334153
2019-01-21 17:06:59r.david.murraysetkeywords: patch, patch, patch, patch

messages: + msg334152
2019-01-21 16:46:55samuelcolvinsetmessages: + msg334149
2019-01-21 16:39:08r.david.murraysetkeywords: patch, patch, patch, patch

messages: + msg334148
2019-01-20 15:23:37samuelcolvinsetmessages: + msg334090
2019-01-20 15:12:35r.david.murraysetstatus: open -> closed
type: crash -> behavior
messages: + msg334088

keywords: patch, patch, patch, patch
resolution: wont fix
stage: patch review -> resolved
2019-01-20 15:08:32eamanusetnosy: + eamanu
messages: + msg334086
2019-01-20 12:26:18samuelcolvinsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11384
2019-01-20 12:26:10samuelcolvinsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11383
2019-01-20 12:26:03samuelcolvinsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11382
2019-01-20 12:25:55samuelcolvinsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11381
2019-01-20 11:14:33samuelcolvincreate