-
-
Notifications
You must be signed in to change notification settings - Fork 32k
bpo-42044: Write all bytes to the console in unbuffered mode on Windows #26678
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
base: main
Are you sure you want to change the base?
Conversation
I am only a moderately interested observer, and from the information in issue 11395 it seems to be fair to remove the capping, now that Windows 7 is not supported anymore. But: When a function returns the number of bytes written, as this one does, shouldn’t the caller be fixed to actually handle partial writes? |
I think that the caller can be fixed, but for me that's a separate (and bigger) issue (i.e.: writing of all bytes to the console in an unbuffered mode can be done in a single call in Windows and it's being artificially constrained from working due to historical issues supporting older versions of Windows). As a note, I actually took a look at fixing the caller, but I don't understand some of the decisions in Also, it'd need to be fixed more places --
So, the fix would have change all those places to do the rewrite, but the |
I don’t see the point of leaving a long comment that essentially apologizes for some deleted code. The commit message is supposed to cover that. Instead state that for long strings this will fail (and how — that’s still unclear to me) on Windows 7, but that W7 is not supported. |
/cc @eryksun |
Note: a fix for issue11395 started to write bytes capped at 32k and mandated the caller to handle partial writes, but when the text io flush was called, it only does only a single call in unbuffered mode. This made it so that the bytes unwritten were actually lost (and not printed) in the process. The reason for that was historical to support older versions of Windows. As of Windows 10, this is no longer needed and thus given issue42044, the number of bytes written to the console is no longer capped.
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.
Much better, but I still don't understand the failure mode. Will it return a correct indicator of how much was written, or will it do something else (then what)?
PS. Once a PR is in review, it's better not to amend your commits -- just add new commits, this is easier for the reviewers. We'll squash when we merge anyway.
It should fail (i.e.: return false and then So, if unable to print it could fail with something as: Note that with this fix this can happen again if printing a string big enough that it wouldn't fit in the current heap memory (vs silently dropping everything above 32KiB which was happening until now). Given that unbuffered was requested, this seems ok to me (but if this limitation is too strong, another option could be doing multiple calls to
Agreed (I thought that this was so small that it'd be ok, but I see how it can be confusing..) |
Windows NT uses local procedure call (LPC) ports for message-based communication between client processes and system components (e.g. subsystems, services, and even kernel components). LPC ports do not directly support arbitrarily large messages. Instead, large messages are exchanged via shared memory, which is typically managed as a heap. Prior to Windows 8, console files are implemented by the console host process (e.g. conhost.exe in Windows 7, and csrss.exe before that). Handle values for console files are tagged by setting the lower two bits (e.g. handle values 3, 7, 11, etc). When This is no longer an issue in Windows 8+. The system console API was redesigned to use a console device instead of LPC. Thus |
For consistency, you can also remove the size limit from |
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.
Callers still have to handle partial writes, so I don't consider this to fix the issue.
But it certainly raises the number of bytes you have to write until it becomes obvious.
Done. |
One thing I considered was changing the semantics so that -- the way things are structured, the actual number of bytes written never gets reported to the initial caller. It just expects that everything has been properly written (for instance, when So, retrying at the caller ( Personally, I think that reporting a failure -- as will happen with this patch -- is a reasonable approach, but if you deem worthy to fix the issue by writing in chunks at |
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 has merge conflicts now.
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 And if you don't make the requested changes, you will be put in the comfy chair! |
https://bugs.python.org/issue42044