Get rid of asynchat_25.py, asyncore_25.py and loggers.py

[palmkevin]

Is there a special reason why supervisor/medusa is using asynchat_25.py, asyncore_25.py and loggers.py instead of their equivalent modules shipped with any standard python release? (asynchat, asyncore and logging)

Is it due to a lack of time of the supervisor developers that these are used or is there any other special reason?

My guess is that nobody wants to venture a too deep trip into medusa?!
BTW: Are there any plans to get rid of (or better: replace) medusa? This project seems quite dead?

[mnaberez]

Is there a special reason why supervisor/medusa is using asynchat_25.py, asyncore_25.py

In Python 2.6, asyncore changed in incompatible ways. We brought this up on python-dev. It became necessary for us to bundle these modules to add support for Python 2.6 while still supporting the earlier versions. Today, Supervisor still supports down to Python 2.4.

and logging)

A few years back, we did extensive profiling throughout Supervisor, and this resulted in various performance improvements. We thought we were spending an unacceptable amount of time in logging, so we rolled our own.

BTW: Are there any plans to get rid of (or better: replace) medusa? This project seems quite dead?

No, not at this time. It's possible we might change it in a later version but there's no incentive right now. Medusa has worked well for Supervisor for years and users of Supervisor are not exposed to it.

[palmkevin]

I understand the reasoning of keeping supervisor compatible with Python 2.4, but I think that time is come to leave this old version behind and create a new non-backward-compatible version of supervisor. (This does not mean that a Py24-compatible (=today's) version cannot be made available for interested people in a separate fork)

IMHO leaving those old dependencies would improve supervisors code quality and increase the project's attractiveness.

[mnaberez]

I understand the reasoning of keeping supervisor compatible with Python 2.4, but I think that
time is come to leave this old version behind and create a new non-backward-compatible version
of supervisor. (This does not mean that a Py24-compatible (=today's) version cannot be made
available for interested people in a separate fork)

We are not going to create two different versions of Supervisor at this time.

Supervisor supported Python 2.3 until a few months ago. It will remain compatible with Python
2.4 for probably another year. We will continue to support it as long as there are a significant
number of users depending on it.

[evanunderscore]

Can we reopen this? Support for 2.5 is no longer a concern.

[evanunderscore]

The incompatibilities mentioned make it non-trivial to swap out the bundled versions for the standard library versions.

[evanunderscore]

I'm fairly convinced that medusa is at the root of a lot of the Python 3 porting issues. The package itself was written for Python 2, and we're trying to use it mostly as-is but with text_socket.py faking strings into and out of sockets, which breaks Python 3's asynchat and asyncore. Porting medusa to deal with bytes properly is going to be rough, since it has no tests (that I can see). Are there any newer alternatives that could be considered?

[evanunderscore]

In the case that we decide to stick with medusa, here's my take on the steps needed if we were to properly port it to Python 3:

1. Write tests for medusa
2. Write tests for the pieces of supervisor that directly use medusa
3. Update bundled versions of asynchat/asyncore to include the Python 2.6 changes
4. Remove text_socket
5. Remove the bundled versions of asynchat/asyncore entirely
6. Update medusa to use bytes where sensible instead of native str
7. Update supervisor where necessary

1 and 2 are needed before any of this code is touched, and 3-7 all have somewhat interleaved dependencies on each other.

Given the amount of work this would take, it seems more sensible to me to replace medusa entirely. Either way this is more work than I'm willing to do at this stage, but hopefully this is of use to anyone else wanting to help out.

[vsajip]

I think the above comments by @evanunderscore have nailed it - the use of text_socket seems like "the wrong fix", in particular. The async{ore,hat} modules seemed very neat when they first came to my notice, but they have some design flaws which I ran into when trying to extend smtpd for use in the stdlib tests for logging. These flaws limit the extensibility of the code therein.

Note that python-dev has deprecated async{ore,hat} in the Python 3.6 documentation, in favour of usage of asyncio - GvR himself raised the Python issue. I think the modules will be around for a while, and Supervisor can't easily use asyncio for a while. The single-threaded, async functionality they provide does seem useful. but whatever is done now to rectify any Supervisor issues in this area would appear to be a stopgap, with asyncio eventually taking the place of these modules.

Barry Warsaw said in another Python issue that

asyncore/chat and smtpd are not the easiest modules to work with, extend, understand, or debug.

which seems to the point.

[mnaberez]

I think the above comments by @evanunderscore have nailed it - the use of text_socket seems like "the wrong fix", in particular.

I also suspect that text_socket may be the wrong approach and #471 is an indication of that. I think one of the issues going on with that is text_socket has converted bytes data to strings, and #471 is trying to convert it back. I don't think we can blanket convert all socket data to strings without introducing issues. Only the consumer of the socket can know if it needs the data as bytes or strings.

[vsajip]

I think a new approach is needed. I think the following steps should be attempted:

1. Make text_socket a no-op by changing it inside the module to always be bin_socket, even under Python 3.x, and make everything work with that.
2. Remove all references to text_socket in favour of socket.socket.

Thoughts?

[evanunderscore]

That approach sounds good to me if you're able to make it work. I tried, but gave up because medusa doesn't appear to have any tests (other than the bits that are tested indirectly by other things), and the interactions between text_socket, async* and the rest of the code made it difficult to make incremental progress without leaving everything in a broken state.

[vsajip]

I've more or less got it working as regards the existing issues labelled "python 3" - that's to say, simple smoke tests work. Looking at adding some tests to cover these specific issues.

All the unit tests pass on 2.x and 3.x with my changes, which isn't perhaps saying all that much.The changes are not in my fix-638 branch, because I thought I would start anew with the approach I described above.

See this Gist for the current working diff. You might want to try applying to current master and having a play with it to see if you can flush out any problems.

The tests have needed a lot of changes because of their fine granularity - they are useful but ultimately not as useful as functional tests, which are a bit thin on the ground here. I've had to do quite a bit of '' → b'' edits, a lot of them in the tests 😞

[evanunderscore]

Nice! How difficult do you think it will be to include those test cases in the automated test suite?

[vsajip]

I tried for one or two, using subprocess, with limited success. The problem is the interlocks between the independent supervisord and supervisorctl processes: apart from adding arbitrary time.sleep calls, I'm not sure how to tell if supervisord has spun up all its child processes and is ready for testing. Perhaps via XMLRPC from the test itself - not sure. Do you have any ideas?

Also, interacting with the supervisorctl via its streams (for the fg and tail -f functionality), while doable, is likely to be tricky and a tad fiddly.

[evanunderscore]

Perhaps pexpect would make things a little nicer than subprocess? Alternatively someone with more experience with this code base might have better ideas.

[evanunderscore]

This gist turns one of your examples into a pexpect test. You should be able to apply it on top of your diff.

$ python3 -m supervisor.tests.test_end_to_end -v
test_issue_565 (__main__.TestEndToEnd) ... ok

----------------------------------------------------------------------
Ran 1 test in 3.464s

OK

Note that it fails in Python 2 because of an actual bug.

[vsajip]

I was just trying to avoid adding a dependency ... but as it's only for test, that shouldn't be too problematic ...

[evanunderscore]

I've briefly looked over the diff - everything looks great! Once your TODOs are addressed and we have tests to verify the known issues are fixed I think we should merge this.

A couple of minor questions:

• Why did this change?
• It looks like this is missing a b prefix

[vsajip]

Re. the missing b prefix - thanks, added.
Re. why %r was changed to \'%s\' - because on Python 2.x, %r sticks an ugly u prefix in the repr, which moreover causes the test to fail.

Re. the end-to-end test failure on Python 2.x - that's more involved. It hinges on code in process.py using str(event) to get the event payload, via overriding __str__ - however, this is ambiguous because it should always be text, but it's different types on 2.x (bytes) and 3.x (str), and __str__ should always return the native string type. When __str__ returns an unexpected type, both 2.x and 3.x fail in different ways - 3.x always fails with an explicit error message about the return type of __str__, and 2.x fails sometimes via a supposedly-helpful implicit encoding the return value of __str__ using ASCII (which of course can fail sometimes).

Fun and games! I think it's better not to make __str__ do this kind of service - perhaps have a method or property on the event such as payload which always returns Unicode (as it will eventually end up in a formatted text message).

[vsajip]

I've updated the Gist to add your end-to-end tests and the others, too, with their fixture data. At the moment I get consistent failures on 2.x and 3.x for test_issue_664 and test_issue_836. It might be something I'm doing wrong in the tests - I'm not very familiar with pexpect. Care to try this on your machine and see if you can see why the failures happen? Things seem to work as expected when run manually.

[evanunderscore]

I forgot about the u prefix - that makes sense. Your current solution will look weird if the text contains a ', but I don't know if that matters. I agree with you that using __str__ is probably wrong, especially since it looks like that could contain Unicode characters.

For why you're getting failures - it's a timing problem. In test_issue_565, I start supervisord, wait for the success message, then start supervisorctl. In test_issue_664 and test_issue_836, you're starting supervisord and supervisorctl at about the same time, which is causing supervisorctl to crash. If you let the exception propagate, you can see this in part of its output: before (last 100 chars): 'http://127.0.0.1:9001 refused connection\r\n'. Additionally, test_issue_836 should use sendline rather than send, and for completeness should probably expect to see the text echoed back to itself. (It seems to match on its own input as well as output, which is odd, but means you'll have to expect the same thing twice.)

[vsajip]

D'Oh! Thanks for pointing out what I should have seen on the pexpect bits. Gist updated, and the improved tests now all pass on 2.x and 3.x :-) Thanks for the help, and have a good weekend!

[evanunderscore]

Thanks, you too!