Catchall 'encoding needs to get un-crap-ified' ticket

[bitprophet]

Background

This is a follow-on from #289 and also encompasses some or all of #241, #274, #262, #332, #242, #321, and #338. My goal is to make this general story better, ASAP, while leaving remaining work clearly defined (regardless of who ends up taking a swing at it).

I need to doublecheck some of those linked issues that I've not spent a lot of time on, but the bulk of discussion is in #289 and #274, much of it driven by @pfmoore who (as a Python 3 on Windows user) serves as a very competent & useful counterweight to myself (a default-Python-2 on Unix developer). Any solution that works for both of us is likely one that serves most users.

The basics that fall out of what's been said there so far:

• Where & how bytes/strings are decoded/encoded, needs to be better defined/documented/contracted;
• Encoding/decoding should be pushed as far out as possible, to the "edges" of the code, to limit how often it happens & becomes an issue;
  • But also centralized further - the code handling it needs to be clearly labeled & no faffing about should happen outside that one area.
• The default behavior should encompass as many use cases as is reasonable to do, and should degrade as gracefully as it can otherwise (re: exceptions, vs replacements, vs ???);
  • But users should also be given the ability to work around that default cleanly & fully whenever it doesn't function for their use case.
  • Ideally, we'll balance these two things well instead of using "the user can twiddle the knobs we provided" as an excuse to let the default behavior suffer. But this is hard and time is short, so...

TODO

• Triple check existing locations where we explicitly or implicitly encode/decode, and move them all somewhere centralized & obvious (without otherwise altering them).
  • Right now the only explicit mentions of (encode|decode)\( are a bare handful in runner.py, so there's likely others that are implicit instead.
  • Not sure what the "most obvious" place is, but given most of this is directly tied to run() and friends, it may still just live in the Runner class or runner.py generally.
• Identify & implement additional tests (integration or regular) for use cases not represented by the funky_characters_in_stdout integration tests
  • Those tests should have caught some of the basic e.g. cat tree -> kaboom bugs that snuck in during 0.11.x, but did not.
  • I identified the basic reason why & have updated it so they fail again, in a branch.
  • But they only represent a single slice of the problem - look at the discussion in Fixed a number of encoding issues (in run context) #274, as well as the other top level issues above, and see what other tests can be added that currently fail.
  • Ideally at least some of these would go in the regular, not integration, suite.
• Get those tests passing, either based off of Fixed a number of encoding issues (in run context) #274 or with something new
• If not implied in above: probably makes sense to add more encoding config toggles (maybe up to "one for each stream") so users can override them granularly; the single encoding is nice and all, but won't work in cases where e.g. one's invoke-invoking terminal is Encoding A but the environment the subprocess is operating in is using Encoding B.
  • Made this Split up encoding settings & defaults to differentiate local vs subprocess #353
• Handle, or surface into this or other tickets, any TODOs added in the branch left undone
• See if @pfmoore has time for immediate feedback to those changes (sadly I wasn't able to give him a thumbs up for his offer of making more PRs on his end, back in Feb) just in case.
• Re: comments below, update read_proc_output so we always expect bytes:
  • Enhance docstring(s) to make it clear that any file-like objects given explicitly, need to yield bytes
  • Remove the "if" statement
  • Update our test suites to use BytesIO instead of StringIO (& any related string literal changes, or maybe a wrapper if lazy).
• Update changelog entry for any fully-closed tickets & their authors

[pfmoore]

Just to tick off one of your TODO items, I should be able to provide feedback on changes when you need it.

[bitprophet]

Catalog of encoding/decoding/etc:

• The bulk of the machinery is currently in Runner._handle_output(), which uses self.encode as well as codecs.iterdecode on read stdout/err data, before writing it to out_stream;
• Another call to self.encode is in handle_stdin (when writing read stdin to the subprocess);
  • Also notable is how handle_stdin only encodes when writing mirrored stdin into the subprocess, but not when echoing back to local stdout. This is one example of the confusion & inconsistency around round-tripping decode/encode vs writing raw read bytes.
• And the last is in respond, because it too writes to the subprocess' stdin;

And a catalog of where we read or write data (i.e. our actual external-facing bits):

• write_stdin uses os.write to write various things (depending on where it is called - right now is either mirrored local stdin, or auto-responses) to the subprocess' stdin fd;
• _handle_output (called by handle_stdout/err) uses <output obj>.write to write the read subprocess' stdout/err to our configured stdout/err stream objects;
• handle_stdin uses <output obj>.write to write (echoing our locally read stdin data) to our configured stdout stream object;
• handle_stdin uses platform.read_byte (which currently uses <input obj>.read but has had some history; if it remains simply .read it can be moved back out of platform probably) to read our own stdin;
• read_stdout/read_stderr use os.read to read our subprocess' stdout/err;

[bitprophet]

Spitballing how to reorganize that without going crazy overboard (e.g. ripping out into a mixin class or whatever):

• Probably best to rename all of the methods regarding WHAT EXACTLY they read/write, e.g. read_proc_stdin, write_own_stdout (or write_local_stdout or write_our_stdout or ???)
• handle_* is a tiny bit harder because they're responsible for more than just reading/writing; but no real reason not to split out the act of reading into its own tiny method?
• Once we have the read/write actions 100% corralled in their own methods, it should presumably be easy to migrate the encoding bits there and only there.
  • Though not really because in some places (like _handle_output, reading+encoding involves multiple iterators
  • Regardless, even if we say "ok, read_proc_stdout is actually an iterator", still helpful to break things up this way.
• Then, hopefully, "fixing" things (now or in future) would involve modifying those specific bits and no more.

[bitprophet]

As I go through the reorg (partly renaming methods to have proc or our inserted; partly creating new such methods where none existed previously), there's one more angle that I recall noticing before, and it's still annoying now:

• reading from the subprocess' out/err naturally has 1 step of indirection due to how it's refactored (out/err behave the same besides the literal read step), and this means there's a nice separation of "do actual read" (implemented in subclasses) and "do other things to the data before letting other things touch it", like decoding bytes->str/unicode (implemented in base class).
• writing to the subprocess' stdin does not have this indirection, but wants it, because otherwise every subclass has to remember to decode, which feels like unacceptable foot-shooting potential.

This isn't really a big deal (besides coming up with non stupid names for the two methods) but I wanted to jot it down anyway 😛

[bitprophet]

Think things are better organized now in my branch, certainly it seems that way - it's made the whole "decode on ingress, encode on egress, it's Just Strings™ in between" angle clearer for sure. Including exposing how silly Runner.encode is at present (encode! but only sometimes! and it's actually bytes->strings, which is decoding, not encoding! [I think!]).

Ditto some tests - by splitting out some read/write bits, it means some tests can now focus solely on the overall behavior ("I expect stdout to be emitted when X is true, but not when Y istrue") and not care as hard about forcing the test fixtures to be bytes-or-strings. (Though most of the tests right now are one way or the other & will need updating when I start actually changing behavior.)

FTR the methods as they now stand are, as expected, representing a simple read/write vs our own pipes / the subprocess' pipes, matrix:

• read_proc_output reads process stdout or err - parameterized as to which stream, via...
• read_proc_stdout/read_proc_stderr, the actual process stdout/err readers
• write_proc_stdin
• write_our_output (parameterized as to which stream, tho not via methods because no need to subclass for this end of things)
• read_our_stdin

[bitprophet]

Pushed what I have so far: https://github.com/pyinvoke/invoke/compare/oh-boy-encoding - not done poking yet (haven't made new tests, only got most existing ones passing again). Basic contents:

• Abovementioned reorg of methods-dealing-with-streams.
• Where we read bytes (from our own stdin, or from subprocess' stdout/err), it is .decode()d, unless already a string.
  • This is also true even when "our stdin" is a user-specified stream-like object; because of the isinstance check, everything is "normalized" to string-like regardless of what's being served up.
• Where we write strings to the subprocess' stdin, it is .encode()d, always;
• Where we write strings to our own stdout/err, it is not encoded, because system streams are io.TextIOWrappers and those want strings, not bytes, so they can do their own encoding and whatnot.
  • Similar to above, when "our stdout/err" are stream-like objects such as StringIO, such things also want strings-not-bytes, so again, differentiating isn't useful.
  • This is one spot where my mental model going in was wrong, I was assuming we'd always be encoding on output. But no, only on the subprocess-facing side, not locally. As it turns out, this is nicely symmetric with how we read, so I think it's good...

Things of special note:

• I have, for now, removed the iterdecode that @pfmoore added back in Fix encoding errors with stdout capture and echoing #211 or so, replacing it with a hopefully-equivalent .decode.
  • At the time it was added, it looks like that was done to massage the result of overly enthusiastic encoding my earlier code did beforehand (so things were being encoded, then decoded...). But I didn't find a super clear answer when I looked. If we reinstate it I deffo want to attach a clear comment nearby...
  • Now that things are a bit more straightened out, it didn't seem necessary (& I confess I never quite grokked why we had to iterdecode and not just decode - again, wish I had put in a comment), thus the 'side-grade' to .decode.
• A few tests used to check if "iterdecode is being given the expected encoding parameter", which now breaks because we're not using iterdecode - and it's harder to test method calls on arbitrary bytes/chars than a mocked imported module.
  • However I think I can modify the test framework to more tightly control/mock that side of things (right now it's simply a StringIO standing in for real streams) so we'll see.
• The integration tests using tree output still fail under Python 2 (this hasn't changed, in other words. Grump!)

[bitprophet]

@pfmoore as noted I've still got work I can do on my end, but if you have early feedback on current state of the branch, let me know. Betting I've made things worse for some use cases so far because I haven't banged the tires hard enough yet. But hoping that at least the reorganization is useful.

[bitprophet]

Also really wondering if there's existing libs out there that solve this ("I have something that's probably raw bytes, gimme a string, I don't really care how") in an easily reusable way. Betting most people just re-solve this time and time again & most solutions are, if clean, still buried in unrelated codebases. Meh.

That said, it's better for me personally that all this gets sorted out, understood, & documented so it can be moved past.

[bitprophet]

Fixed the tests that were caring about the encoding settings - good enough for now at least.

The integration tests are still failing & clearly also the crux of some of the real world issues. Basic stab:

• Part of the problem may be the previously identified problems with locale.getpreferredencoding(False) turning up the wrong encoding.

  • E.g. I'm running it in a terminal with LANG=en_US.UTF-8, and sys.stdout.encoding (on Python 2) is also correctly reporting UTF-8.
  • But locale.getpreferredencoding(False) returns US-ASCII, which is wrong; changing to no param or True gets UTF-8. So that's one thing we need to figure out (gotta double check what's already been said about this in Fixed a number of encoding issues (in run context) #274 and elsewhere)

• Another is the discrepancy between print() and sys.stdout.write() under Python 2:

  • Taking a Unicode string with non-ASCII escapes in it and print()ing it works OK, but the exact same string sent to sys.stdout.write() yields UnicodeEncodeError: 'ascii' codec can't encode characters in position 18-20: ordinal not in range(128).
  • However, running encode on that variable and then handing it to sys.stdout.write, works OK (and the same works OK for print too). Trivial example of this (\u251c is the first non-ASCII-friendly character in the data we read from the subprocess running cat tree.out):

  >>> x = u"\u251c"
  >>> x
  u'\u251c'
  >>> print(x)
  ├
  >>> x.encode('utf-8')                                                                                                                                       
  '\xe2\x94\x9c'
  >>> print(x.encode('utf-8'))                                                                                                                                
  ├
  >>> import sys
  >>> sys.stdout.write(x)
  Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  UnicodeEncodeError: 'ascii' codec can't encode character u'\u251c' in position 0: ordinal not in range(128)
  >>> sys.stdout.write(x.encode('utf-8'))
  ├
  

  • This - the apparent need to encode before sys.stdout.writeing - seems to contradict what I found just previously re: sys.stdout.write getting pissy if you give it a bytes. But that appears to only be Python 3 - so that's the crux there. (Why do I feel like this is all a huge fricken retread...)

[pfmoore]

@bitprophet I'll try to make some time over this weekend to review all this.

The problem with sys.stdout.write and Unicode is likely because under Python 2, sys.stdout writes str (== bytes) objects, not Unicode, and so needs to encode them. But if you don't specify an encoding in Python 2, the default for unicode->bytes conversion is ASCII. On Python 3, sys.stdout is still a str stream, but this now means Unicode, not bytes...

I suspect the best solution is to have a couple of Python 2/3 compatibility functions, write_bytes_to_stdout and write_unicode_to_stdout and use them exclusively, rather than ever using sys.stdout.write directly. (You may prefer to make the functions take a general file object, rather than hard coding stdout but the idea remains the same).

That's just off the top of my head though - I'll give more detailed comments once I've looked at the code.

[pfmoore]

BTW, the point about iterdecode was that we were getting bytes from the get() function. And because therewas no guarantee that I could find that get() wouldn't split a multi-byte sequence part way through, we need to use iterdecode to "hold onto" the partly-complete sequence ready for the next call.

Consider

>>> 'abc£def'.encode('utf-8')[:4].decode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc2 in position 3: unexpected end of data

By splitting the bytes at byte 4, we cut the 2-byte sequence for "£" in half, and decode fails. That's precisely the situation iterdecode is designed to deal with.

It's also an annoying issue, because it's really unlikely to come up with in practice, so the bug could lie dormant for ages. Might be worth explicitly constructing a test to make sure this is handled.

[bitprophet]

Re: sys.stdout.write: yea I assumed we'd need some intermediate code handling this, glad to hear we're on the same page there, thanks.

Re: iterdecode: I actually had thoughts about what you mention, but in the spot where we read from our local stdin one byte at a time, there's a TODO worrying about multibyte characters. Totally acknowledge that it's the same problem, just much less likely to happen, with reading stdout (currently/by default, at 1000 bytes/read, instead of 1...).

About the stdout bit which used to use iterdecode, I don't recall whether the get iterator was necessary beforehand, but there isn't one now. Feels like we may need to just roll back to what was there before, except not having that earlier/apparently-unnecessary .encode. I'll have to poke some.

[bitprophet]

Note to self, another good real world ish encoding-is-fucked test is Debian's /usr/share/dict/words (as seen over in #350) - on Linux (but not on Darwin) it is UTF-8 text with a handful of words containing é at the end, which - when catted inside an Invoke run - turn up as ye olde question mark.