GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-87597: Decode subprocess output in text mode when timeout is hit by LewisGaul · Pull Request #95579 · python/cpython

Conversation

LewisGaul commented Aug 2, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

LewisGaul requested a review from gpshead as a code owner

August 2, 2022 18:01

ghost commented Aug 2, 2022 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

All commit authors signed the Contributor License Agreement.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting review label

Aug 2, 2022

LewisGaul added 2 commits

August 2, 2022 19:05


          Implement handling of unicode decode error from incomplete code bytes

fa8d19c


          Add testcase

aecc55b

LewisGaul force-pushed the fix-issue-87597 branch from e5f4ee1 to aecc55b Compare

August 2, 2022 18:05


          📜🤖 Added by blurb_it.

4b4b7eb

LewisGaul commented

Aug 2, 2022

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2022-08-02-18-12-34.gh-issue-87597.UHFR0H.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

LewisGaul added 2 commits

August 2, 2022 21:23


          Update Misc/NEWS.d/next/Core and Builtins/2022-08-02-18-12-34.gh-issu…

c4b1012

…e-87597.UHFR0H.rst


          Skip testcase on Windows - after timeout the threads reading output a…

a2c7e06

…re left running

LewisGaul commented Aug 3, 2022

Copy link

Copy Markdown

Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun reviewed

Aug 3, 2022

View reviewed changes

Comment thread

Lib/test/test_subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

LewisGaul added 3 commits

August 3, 2022 23:22


          Test markups

9d2168e


          If no output before timeout is hit ensure empty string/bytes is store…

98bff39

…d rather than None


          Reduce timeout when not waiting for any output

39393a8

LewisGaul mentioned this pull request

Aug 7, 2022

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

Open


          Merge branch 'main' into fix-issue-87597

34dfe5a

LewisGaul commented Aug 15, 2022

Copy link

Copy Markdown

Contributor Author

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eryksun added awaiting core review stdlib

Standard Library Python modules in the Lib/ directory

3.12

only security fixes

3.11

only security fixes

3.10 (EOL) end of life labels

Aug 15, 2022

zooba reviewed

Sep 30, 2022

View reviewed changes

Comment thread

Lib/subprocess.py

		encoding,
		errors)
		else:
		raise

zooba Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/subprocess.py

Comment on lines +1265 to +1271

		if stdout_seq is not None:
		stdout = b''.join(stdout_seq)
		if self.text_mode:
		stdout = translate_newlines_partial_output(
		stdout, self.stdout.encoding, self.stdout.errors)
		else:
		stdout = None

zooba Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead requested changes

Sep 30, 2022

View reviewed changes

Comment thread

Lib/subprocess.py

		encoding,
		errors)
		else:
		raise

gpshead Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/subprocess.py

Comment on lines +1265 to +1271

		if stdout_seq is not None:
		stdout = b''.join(stdout_seq)
		if self.text_mode:
		stdout = translate_newlines_partial_output(
		stdout, self.stdout.encoding, self.stdout.errors)
		else:
		stdout = None

gpshead Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_subprocess.py

		stdout, stderr = popen.communicate(input='')
		self.assertEqual(stdout, '1\n2\n3\n4')

		@unittest.skipIf(mswindows, "behavior currently not supported on Windows")

gpshead Sep 30, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zooba Oct 3, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting changes and removed awaiting review awaiting core review labels

Sep 30, 2022

bedevere-bot commented Sep 30, 2022

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead self-assigned this

Sep 30, 2022

gpshead commented Sep 30, 2022

Copy link

Copy Markdown

Member

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.

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba commented Oct 3, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead removed 3.11

only security fixes

3.10 (EOL) end of life labels

May 20, 2023

gpshead marked this pull request as draft

May 20, 2023 23:51

bedevere-bot removed the awaiting changes label

May 20, 2023

gpshead removed their assignment

May 20, 2023

This was referenced Mar 22, 2026

feat: add probabilistic prediction planner modes ll7/robot_sf_ll7#686

Merged

Add minimal Python MCP recon server with fixed nmap/curl/whois tools Chemically-Motivated-Solutions/promptcrafting-mcp#52

Merged

github-actions Bot commented Apr 11, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 11, 2026

coderabbitai Bot mentioned this pull request

May 12, 2026

Implement IQRA sandbox orchestrator and update skills with Python implementations AIX-Format/aix-agent-skills#3

Merged

coderabbitai Bot mentioned this pull request

Aug 4, 2026

Fix place_many row-shape ergonomics and protect the MCP stdio channel leynos/voxel-forge#9

Open

zware removed the 3.12 only security fixes label

Oct 1, 2026

github-actions Bot removed the stale Stale PR or inactive for long period of time. label

Oct 6, 2026

Reviewers

eryksun

eryksun left review comments

zooba

zooba left review comments

gpshead

gpshead requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

stdlib

Standard Library Python modules in the Lib/ directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

LewisGaul commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 2, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LewisGaul commented Aug 3, 2022

Uh oh!

Uh oh!

LewisGaul commented Aug 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 30, 2022

Uh oh!

gpshead commented Sep 30, 2022

Uh oh!

zooba commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LewisGaul commented Aug 2, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

ghost commented Aug 2, 2022 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

zooba commented Oct 3, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.