Skip to content

Initial cut for fix of issue #1231 #1235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 22, 2019
Merged

Conversation

vsajip
Copy link
Contributor

@vsajip vsajip commented May 1, 2019

I've added a warning in the tailf command, and also changed the exception raised on UnicodeEncodeError to have a more informative message. A couple of related new end-to-end tests have been added and on my Ubuntu system, all tests pass with tox on all configured platforms.

@vsajip
Copy link
Contributor Author

vsajip commented May 1, 2019

OK, I see failures on Travis. This could be because the encoding when LANG=oops is environment-dependent :-( @mnaberez can you test this locally and see what happens?

In several of the failures, it doesn't even get around to running the child process. I can't restart the Travis jobs, though.

@vsajip
Copy link
Contributor Author

vsajip commented May 9, 2019

OK, I had some time to look at this again. The .conf file used in the test was left out of the commit because of a .gitignore rule. I added the file and now all the tests pass :-)

@mnaberez do you want to take a look and see if you think it's ready to merge?

@mnaberez
Copy link
Member

@mnaberez can you test this locally and see what happens?

I tested it on Python 2.7.15 and 3.6.7:

Python 2.7.15 tail

$ LANG=en_US.UTF-8 supervisorctl tail geth
] delay= 668 hash=57d94b…381088
INFO [05-17|09:23:01] delay=1141 hash=57d94b…381088
INFO [05-17|09:23:02] delay= 413 hash=57d94b…381088
INFO [05-17|09:23:03] delay= 915 hash=57d94b…381088
$ LANG=oops supervisorctl tail geth
] delay= 777 hash=57d94b…381088
INFO [05-17|09:23:32] delay= 965 hash=57d94b…381088
INFO [05-17|09:23:33] delay= 595 hash=57d94b…381088
INFO [05-17|09:23:34] delay=1083 hash=57d94b…381088

Python 2.7.15 tail -f

$ LANG=en_US.UTF-8 supervisorctl tail -f geth
==> Press Ctrl-C to exit <==
FO [05-17|09:21:53] delay= 899 hash=57d94b…381088
INFO [05-17|09:21:53] delay= 835 hash=57d94b…381088
INFO [05-17|09:21:54] delay= 764 hash=57d94b…381088
INFO [05-17|09:21:55] delay= 887 hash=57d94b…381088
$ LANG=oops supervisorctl tail -f geth
Warning: sys.stdout.encoding is set to US-ASCII, so Unicode output may fail.
==> Press Ctrl-C to exit <==
FO [05-17|09:22:17] delay=1177 hash=57d94b…381088
INFO [05-17|09:22:18] delay= 732 hash=57d94b…381088
INFO [05-17|09:22:19] delay= 889 hash=57d94b…381088
INFO [05-17|09:22:20] delay=1044 hash=57d94b…381088

Python 3.6.7 tail

$ LANG=en_US.UTF-8 supervisorctl tail geth
] delay=1058 hash=57d94b…381088
INFO [05-17|10:04:22] delay= 672 hash=57d94b…381088
INFO [05-17|10:04:23] delay=1027 hash=57d94b…381088
INFO [05-17|10:04:24] delay= 564 hash=57d94b…381088
$ LANG=oops supervisorctl tail geth
error: <class 'UnicodeEncodeError'>, 'ascii' codec can't encode character '\u2026' in position 24: ordinal not in range(128): file: /usr/local/lib/python3.6/dist-packages/supervisor-4.0.3.dev0-py3.6.egg/supervisor/supervisorctl.py line: 244

Python 3.6.7 tail -f

$ LANG=en_US.UTF-8 supervisorctl tail -f geth
==> Press Ctrl-C to exit <==
FO [05-17|10:05:14] delay= 473 hash=57d94b…381088
INFO [05-17|10:05:14] delay= 400 hash=57d94b…381088
INFO [05-17|10:05:15] delay= 498 hash=57d94b…381088
INFO [05-17|10:05:15] delay= 625 hash=57d94b…381088
$ LANG=oops supervisorctl tail -f geth
Warning: sys.stdout.encoding is set to ANSI_X3.4-1968, so Unicode output may fail.
==> Press Ctrl-C to exit <==
unix:///path/to/supervisor.sock/logtail/geth/stdout Cannot connect, error: <class 'ValueError'> (Unable to write Unicode to stdout because it has encoding ANSI_X3.4-1968)

@mnaberez
Copy link
Member

mnaberez commented May 17, 2019

@mnaberez do you want to take a look and see if you think it's ready to merge?

  • Python 2.7.15 tail and tail -f work for all cases but the new warning is only printed for tail -f
  • Python 3.6.7 fails the LANG=oops cases for both tail and tail -f as we expected it would but
    the new warning is only printed for tail -f.

Thanks for this patch! The original issue in #1231 (tail -f fails on Python 2) is confirmed to be fixed. The patch looks good and I appreciate that you added tests. I have only this feedback:

Since both tail and tail -f can fail, I think we should print the warning on both (or at startup). Currently it only prints the warning on tail -f. I would also add some hint to the warning about
environment variables to give the user some initial direction to fix it.

Please let me know if you would be able to make that change. Either way, I would like to merge this and make a point release soon to fix the Python 2 issue. Thanks again.

@mnaberez
Copy link
Member

Merging. We can always update the warning message later. Thank you!

@mnaberez mnaberez merged commit da0b323 into Supervisor:master May 22, 2019
mnaberez added a commit that referenced this pull request May 22, 2019
@plockaby
Copy link
Contributor

plockaby commented May 22, 2019

Likely too late to make this change and probably not something that could be done on a minor point release but since I spent a ton of time on this just yesterday...

Migrating from version 3 to version 4, my event listeners read from supervisor over stdin using a byte stream and then did the conversion to utf8 and chose to "backslashreplace" undecodable utf8 data. When I went to version 4 I expected to continue to get a byte stream that I could convert myself to whatever encoding I knew the data to be in. It was really unexpected to discover that supervisor was doing the conversion for me and then telling me the number of characters to read off of stdin and not the number of bytes.

So I wish that supervisor would leave the encoding decision (and what to do when the data doesn't decode correctly) to the client and not do it for me. But I'm sure that decision is too late to be changed, especially in a point release.

@mnaberez
Copy link
Member

@plockaby I don't think that's related to this issue (supervisorctl tail -f) so I created #1243 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants