gh-87597: Decode subprocess output in text mode when timeout is hit

[LewisGaul]

When using text=True and a timeout is hit from subprocess.run(cmd, timeout=T, text=True, capture_output=True) then the resulting subprocess.TimeoutExpired exception incorrectly stores stdout and stderr in bytes, or if no output was received sets the attributes to None rather than the empty string.

This results in the need for ugly workarounds such as:

try:
    p: subprocess.CompletedProcess[str] = subprocess.run(cmd, **kwargs)
except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e:
    if isinstance(e, subprocess.TimeoutExpired):
        # Workaround for https://github.com/python/cpython/issues/87597,
        # TimeoutExpired gives bytes or None rather than str.
        if isinstance(e.stdout, bytes):
            e.stdout = e.stdout.decode("utf-8")
        if e.stdout is None:
            e.stdout = ""
        if isinstance(e.stderr, bytes):
            e.stderr = e.stderr.decode("utf-8")
        if e.stderr is None:
            e.stderr = ""
    logger.error(
        "Command failed with stdout:\n%s\nstderr:\n%s", e.stdout, e.stderr
    )

The complexity is around the fact that the subprocess that was timed out may have given partial output that cannot be decoded (only some bytes from a codepoint), but this can be handled by ignoring a partial trailing codepoint.

Credit to @JessToudic for the suggestion of using the info already available on the UnicodeDecodeError exception and for helping to reproduce the issue/come up with the fix!

@eryksun, @macdjord (participants on the issue)

• Issue: Subprocess timeout causes output to be returned as bytes in text mode #87597

[ghost]

All commit authors signed the Contributor License Agreement.

[LewisGaul]

I'm not sure why this is failing on MacOS, I would've expected it to follow the posix code paths, but I don't know much about mac... Any suggestions?

[LewisGaul]

Any chance of a review @eryksun? :)

Do we think the fix should be backported, since it's marked as a bug? It would be great to get this in 3.9 so I can remove a workaround I currently have in place!

[zooba]

This exception would be raised instead of the timeout error, which isn't going to be helpful. I considered three alternatives, but I think the last is the best:

• start raising the TimeoutExpired, then raise the UnicodeDecodeError from it
• defer decoding to the TimeoutExpired class (so it learns to keep encoding/errors around and do the decode on demand)
• decode up to exc.start here, then decode the rest with the replace error handler and concatenate it

[gpshead]

Agreed, I like that latter strategy. I think it is important to have the TimeoutExpired be the primary error. Otherwise the caller could wrongly interpret the UnicodeDecodeError as the process exiting naturally while producing undecodable output. If the process did emit undecodable data on its own, that isn't important in this situation.

[zooba]

Maybe this whole pattern could go into the helper function? join_and_translate_newlines?

[gpshead]

I think the if/else around the None makes sense here as is, but the join and text_mode conditional could be done in-function.

basically

def join_and_maybe_decode(output_seq, data, encoding, errors):
    output = b''.join(output_seq)
    if self.text_mode:
        try:
            ... # existing translate_newlines_partial_output code

and here you'd have things like

if stdout_seq is not None:
    stdout = join_and_maybe_decode(stdout_seq, data, encoding, errors)
else:
    stdout = None

# and the same for stderr

[gpshead]

Agreed, I like that latter strategy. I think it is important to have the TimeoutExpired be the primary error. Otherwise the caller could wrongly interpret the UnicodeDecodeError as the process exiting naturally while producing undecodable output. If the process did emit undecodable data on its own, that isn't important in this situation.

[gpshead]

I think the if/else around the None makes sense here as is, but the join and text_mode conditional could be done in-function.

basically

def join_and_maybe_decode(output_seq, data, encoding, errors):
    output = b''.join(output_seq)
    if self.text_mode:
        try:
            ... # existing translate_newlines_partial_output code

and here you'd have things like

if stdout_seq is not None:
    stdout = join_and_maybe_decode(stdout_seq, data, encoding, errors)
else:
    stdout = None

# and the same for stderr

[gpshead]

what is needed for this to work on Windows?

having inconsistent behavior in the returned stderr/stdout types on Timeout between the two platforms will make people's code difficult.

[zooba]

Hah, I didn't even notice this.

I'm guessing this should actually be "platforms that don't default to UTF-8 don't trigger the decoding error", which luckily is easily resolved by explicitly specifying that the encoding should be utf-8.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[gpshead]

Do we think the fix should be backported, since it's marked as a bug? It would be great to get this in 3.9 so I can remove a workaround I currently have in place!

We cannot. I don't even think we can accept this as written today without a deprecation period as this is changing a public API so that makes this a feature.

People have already written code less pedantic than your own isinstance checking example that blindly assumes on TimeoutExpired that the stdout/stderr data is bytes or None. So this is an API change that breaks code in existing releases.

That also means we cannot accept this behavior change for 3.12 and need to modify this PR to either:

• A) Be conditional on yet another subprocess keyword only argument controlling the behavior. Documenting that as added in 3.12 with the default planned to changed in the future. Yet another flag is annoying.

• B) A feature we could ship immediately in 3.12 is adding a function or method to do the potential truncated text decoding with a TimeoutExpired documentation tie in.

Whatever behavior we have needs to become consistent across platforms as well.

[zooba]

I don't even think we can accept this as written today without a deprecation period as this is changing a public API so that makes this a feature.

This is true.

What we might be able to do now is to make TimeoutExpired decode on demand when its (new) decode attribute is True.

try:
    subprocess....
except TimeoutExpired as ex:
    ex.decode = True
    print(ex.stdout) # decode happens here

This is safe enough, because all versions will support setting that attribute, it just won't have any effect on old ones. We can then also show a deprecation warning for accessing the attributes without setting decode and say that being decoded will become the default in 3.14. As long as any other fields we attach are _ prefixed and aren't in .args, we can drop them at that time. We don't even have to set decode to False, so people can't start to rely on reading from it.