You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/05/01 21:24:27 UTC
Re: Review Request 66431: Windows: Fixed `os::read()` to use
`ReadFile()`.
> On April 18, 2018, 11:55 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/read.hpp
> > Lines 39-44 (patched)
> > <https://reviews.apache.org/r/66431/diff/4/?file=1994444#file1994444line41>
> >
> > So you're saying that `ReadFile` reads all the existing data on the handle prior to returning False? It would feel safer if we verify this with a unit test.
>
> Andrew Schwartzmeyer wrote:
> I think that `TEST_F(SubprocessTest, PipeOutputToFileDescriptor)` adequately ensures this is working as inspected. Let me know what you think...
>
> Joseph Wu wrote:
> I don't think piping output to an FD is quite what I'm thinking of.
>
> Perhaps a test should:
> (1) Open a subprocess that writes to a pipe held by the test.
> (2) The subprocess should write a substantial amount of data (more than a memory page).
> (3) Close the subprocess before reading any data from the pipe. Presumably this is where the broken pipe error should occur?
Done, in https://reviews.apache.org/r/66892/
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66431/#review201442
-----------------------------------------------------------
On April 9, 2018, 3:53 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66431/
> -----------------------------------------------------------
>
> (Updated April 9, 2018, 3:53 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8676
> https://issues.apache.org/jira/browse/MESOS-8676
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This can eventually support overlapped I/O.
>
> The Windows API `ReadFile()` returns an error if the pipe is broken,
> where `_read()` did not, but this is not an error for us as the data
> is still read correctly. So we ignore it.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/read.hpp 49878e499209fa2f91fede0ebdabb8f088a9d018
> 3rdparty/stout/include/stout/os/windows/read.hpp 8047ad590fcc46d3ec46b551472d8c518ae49cc1
>
>
> Diff: https://reviews.apache.org/r/66431/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>