Opened 7 years ago

Last modified 6 years ago

#7058 enhancement new

support the stdin,stdout parameters to StandardIO() on windows

Reported by: infinity0 Owned by: infinity0
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/win32-stdio-init-params-7058
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

In windows, file descriptors and file handles are different things, even though they have the same function. To get a file handle, we use msvcrt.get_osfhandle. This is what was missing from the previous code.

Unfortunately have to make the test slightly more complex, to accommodate for the fact that Windows throws an error if we try to close an already-closed fd.

We can re-simplify by doing r = path.open('r') and cutting out w altogether, but I thought I'd leave the existing behaviour in to prove that using os.pipe() also works.

Attachments (3)

stdio-init-params.patch (4.0 KB) - added by infinity0 7 years ago.
stdio-init-params.2.patch (3.9 KB) - added by infinity0 7 years ago.
stdio-init-params.3.patch (4.2 KB) - added by infinity0 7 years ago.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by infinity0

Attachment: stdio-init-params.patch added

comment:1 Changed 7 years ago by infinity0

Keywords: review added

comment:2 Changed 7 years ago by Itamar Turner-Trauring

On POSIX you can pass in file descriptors for stdin/stdout. What would you pass in on Windows? file("c:\\my.txt").fileno()?

comment:3 Changed 7 years ago by infinity0

fileno() *is* the file descriptor, and you would pass it in for both the POSIX and windows StandardIO classes.

though, I just noticed that the POSIX StandardIO class uses 0,1 as default values for stdin,stdout so perhaps I should mirror that to be consistent.

Changed 7 years ago by infinity0

Attachment: stdio-init-params.2.patch added

comment:4 Changed 7 years ago by Itamar Turner-Trauring

So you're saying you'd pass in the file descriptor of filesystem files, yes? Anything else that might be passed in?

comment:5 Changed 7 years ago by infinity0

If you are implying that I should also accept Python file objects here, I believe this would be inappropriate, because the POSIX StandardIO class also does not do this, only accepting file descriptor integers. (It uses ProcessReader/Writer which only accept fd arguments.)

If Twisted wants to support file objects for these arguments, it should apply it to both platforms in a subsequent patch.

comment:6 Changed 7 years ago by Itamar Turner-Trauring

Let me rephrase - why do you need these extra arguments? What particular objects might be passed in? What would the docstring for __init__ say?

comment:7 Changed 7 years ago by infinity0

The answers to these questions, are the same answers as for the POSIX StandardIO, which is already in Twisted. What must I answer, that has not already been answered by the previous committer? Please be more explicit.

comment:8 Changed 7 years ago by Itamar Turner-Trauring

I'm not trying to imply anything or suggest a design, I'm just trying to figure what functionality this patch provides. You pass in numbers - what are those numbers? What OS object are they?

On POSIX, the passed in numbers can be filesystem files, pipes, or perhaps even sockets. I don't know enough about win32 to know what can be passed in, but I do know it's likely to be a different set of things. So I'm trying to understand what the feature exactly enables? What is your use case?

You would need to write a docstring explaining what these values mean - the fact POSIX version doesn't have docstring is a result of it being old code, it violates our coding standard. So what would that docstring say?

Changed 7 years ago by infinity0

Attachment: stdio-init-params.3.patch added

comment:9 Changed 7 years ago by infinity0

http://msdn.microsoft.com/en-us/library/kdfaxaay.aspx https://docs.python.org/2/library/os.html#file-descriptor-operations

On Windows, they represent open files or pipes; sockets don't exist. My use-case is the same as for the POSIX part that is already in Twisted.

I've added this explanation to the docstring.

comment:10 Changed 6 years ago by Glyph

Author: glyph
Branch: branches/win32-stdio-init-params-7058

(In [42907]) Branching to win32-stdio-init-params-7058.

comment:11 Changed 6 years ago by Glyph

(In [42908]) Apply stdio-init-params.3.patch.

Thanks, infinity0!

refs #7058

comment:12 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to infinity0

Thanks again for your contributions and your patience, infinity0.

Unfortunately, It looks like this patch still doesn't have any test coverage. We need test coverage to land it, so could you add some?

If you click on the "buildbot" link at the top of the ticket you will see there are some new pyflakes and twistedchecker errors which you should fix.

In the documentation there should also be a reference to some Microsoft documentation as to what kind of thing a "file descriptor" is. I can't find the relevant documentation myself right now, but there's some name for these things, since they're MSVCRT wrapper artifacts and not handles - this is important because there are also handles which represent the same underlying resources, and that can be really confusing.

Note: See TracTickets for help on using tickets.