Skip to content
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

Fixes an issue on Python3 with our stderr redirecting. #77

Merged
merged 3 commits into from
Jan 18, 2021

Conversation

pscadding
Copy link

@pscadding pscadding commented Dec 15, 2020

I'm unsure if this is the right fix or not.

I understand that in Python 2, doing

se = open("/my_file.txt", "a+", 0)
se.write("hi")
import time
time.sleep(15)

Would mean that the "hi" text would be written to the file before the script ended without a flush because we provided the 0 unbuffered argument.
However in Python 3 this is only available for binary objects. So you can use the following instead:

se = open("/my_file.txt", "a+b", 0)

This would cause an error with the original script because we are passing a string, but if we did:

import os, sys

stderr = "/test.txt"
se = open(stderr, "a+b", 0)
os.dup2(se.fileno(), sys.stderr.fileno())

a = 8/0

That actually is fine and the error is written to the file. I'm unsure why that is OK when an error is generated, I presume it is being converted to a binary?
But also I'm not sure of the importance here of setting the buffer to 0 on an error, presumably it will be flushed as soon as it errors since the program will stop? And even if it didn't then is it important since we are from what I can tell discarding it anyway?

So perhaps this would be fine instead?

se = open("/my_file.txt", "a+")

buffer size 0 is only available for binary objects.
@@ -11,8 +11,7 @@ Fx](http://rodeofx.com) and [Oblique](http://obliquefx.com). It is now part of
initiative](https://github.com/shotgunsoftware).

This software is provided under the MIT License that can be found in the LICENSE
file or at the [Open Source Initiative](http://www.opensource.org/licenses/mit-
license.php) website.
file or at the [Open Source Initiative](http://www.opensource.org/licenses/mit-license.php) website.
Copy link
Author

Choose a reason for hiding this comment

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

I just spotted the link formatting was broken here.

@mathurf mathurf requested a review from eshokrgozar January 7, 2021 17:11
Copy link

@eshokrgozar eshokrgozar left a comment

Choose a reason for hiding this comment

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

Makes sense to me

Copy link

@jfboismenu jfboismenu left a comment

Choose a reason for hiding this comment

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

I have a question. Any idea why we need to do add binary mode to stderr but not stdout? They both point to a file, right, so wouldn't the same issue be present?

@pscadding
Copy link
Author

Yeah I don't know, it's why I posed the question in the PR description. I'm not sure on the importance of the binary mode on the stderr, as it appears to work without it. I erred on the side of not changing it.

@jfboismenu
Copy link

I don't understand what you mean by I'm not sure on the importance of the binary mode on the stderr, as it appears to work without it. I erred on the side of not changing it.

Your PR added the b next to a+, so you did change it. It is the only change. If you removed it there would be no diff beside the comments.

@pscadding
Copy link
Author

sorry, what I meant was that I could have removed the 0 and it would have worked, but I opted to not change that behaviour, and simply preserve that behaviour in Python 3 by adding the b.

@jfboismenu
Copy link

stderr on Python is unbuffered, which means that everything you sent to it is sent out immediately, while stdout is buffered and will only write out something to the screen or a file when the content is too big or a newline is found.

There's a Stackoverflow (obviously) post out this: https://stackoverflow.com/questions/19990589/stderr-and-stdout-not-buffered-vs-buffered

@jfboismenu
Copy link

Obviously, the post is about C, but I'm assuming this holds for Python as well, since it's written in C.

@pscadding
Copy link
Author

yeah I'd read that as well, and understood that it would cause it to be output without needing to be flushed, but in practice it didn't seem to make any difference to the outcome, since on error it would end the script and presumably flush? At least, I didn't appear to have any issues with getting the error output without the binary mode.

@pscadding pscadding merged commit 6f52877 into master Jan 18, 2021
@pscadding pscadding deleted the SG-19983-fix-sdterrr-python3-errror branch January 18, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants