Re: Review Request 66957: Windows: Enabled creating overlapped pipes with `os::pipe`.

2018-05-23 Thread Andrew Schwartzmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66957/#review203691
---


Ship it!




Ship It!

- Andrew Schwartzmeyer


On May 22, 2018, 3:51 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66957/
> ---
> 
> (Updated May 22, 2018, 3:51 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8674
> https://issues.apache.org/jira/browse/MESOS-8674
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::pipe` was using `CreatePipe`, which does not support overlapped
> io. Now, the implementation of `os::pipe` has been changed to use
> `CreateNamedPipe`, which supports overlapped IO. The named pipe is
> created with an unique random name, so it is effectively anonymous.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/pipe.hpp 
> a3574fd6f2ff1608396b47cad8cbed88134a74ca 
> 
> 
> Diff: https://reviews.apache.org/r/66957/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 66957: Windows: Enabled creating overlapped pipes with `os::pipe`.

2018-05-22 Thread Akash Gupta 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66957/
---

(Updated May 22, 2018, 10:51 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
John Kordich, Joseph Wu, and Radhika Jandhyala.


Changes
---

Ran clang-format and updated with Andy's suggestions.


Bugs: MESOS-8674
https://issues.apache.org/jira/browse/MESOS-8674


Repository: mesos


Description
---

`os::pipe` was using `CreatePipe`, which does not support overlapped
io. Now, the implementation of `os::pipe` has been changed to use
`CreateNamedPipe`, which supports overlapped IO. The named pipe is
created with an unique random name, so it is effectively anonymous.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/windows/pipe.hpp 
a3574fd6f2ff1608396b47cad8cbed88134a74ca 


Diff: https://reviews.apache.org/r/66957/diff/2/

Changes: https://reviews.apache.org/r/66957/diff/1-2/


Testing
---


Thanks,

Akash Gupta

Re: Review Request 66957: Windows: Enabled creating overlapped pipes with `os::pipe`.

2018-05-07 Thread Andrew Schwartzmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66957/#review202585
---




3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 35-36 (original), 36-37 (patched)


Why do we choose to default to overlapped here?


- Andrew Schwartzmeyer


On May 4, 2018, 10:20 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66957/
> ---
> 
> (Updated May 4, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8674
> https://issues.apache.org/jira/browse/MESOS-8674
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::pipe` was using `CreatePipe`, which does not support overlapped
> io. Now, the implementation of `os::pipe` has been changed to use
> `CreateNamedPipe`, which supports overlapped IO. The named pipe is
> created with an unique random name, so it is effectively anonymous.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/pipe.hpp 
> a3574fd6f2ff1608396b47cad8cbed88134a74ca 
> 
> 
> Diff: https://reviews.apache.org/r/66957/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 66957: Windows: Enabled creating overlapped pipes with `os::pipe`.

2018-05-07 Thread Andrew Schwartzmeyer 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66957/#review202583
---




3rdparty/stout/include/stout/os/windows/pipe.hpp
Line 32 (original), 33 (patched)


"Overlapped pipes passed to child processes behave weirdly"



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 45 (patched)


Run clang-format again. There's an extra space in `;  i < 10;`



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 45-58 (patched)


This is entirely unncessary. If we can't trust that a UUID is unique, we 
have other problems. This can be replaced with `std::wstring name = 
wide_stringify("\\.\pipe\mesos-pipe-" + id::UUID::random().toString())`.



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 74 (patched)


Do we need to worry about this in the remarks section?

> Windows 10, version 1709:  Pipes are only supported within an 
app-container; ie, from one UWP process to another UWP process that's part of 
the same app.



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 75 (patched)


I've been preferring `.data()` since it's a `wstring`. It just seemed more 
fitting, though they are the same.



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 82 (patched)


Maybe note that this means it's not inheritable.



3rdparty/stout/include/stout/os/windows/pipe.hpp
Line 48 (original), 88-95 (patched)


Maybe note that we're not using `CallNamedPipe` because this is a `BYTE` 
pipe and not a `MESSAGE` pipe / a little expo on Windows named pipes for the 
uninformed reader.



3rdparty/stout/include/stout/os/windows/pipe.hpp
Lines 98-100 (patched)


You can just create the `WindowsError` first, then `CloseHandle`, then 
return the error.


- Andrew Schwartzmeyer


On May 4, 2018, 10:20 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66957/
> ---
> 
> (Updated May 4, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8674
> https://issues.apache.org/jira/browse/MESOS-8674
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::pipe` was using `CreatePipe`, which does not support overlapped
> io. Now, the implementation of `os::pipe` has been changed to use
> `CreateNamedPipe`, which supports overlapped IO. The named pipe is
> created with an unique random name, so it is effectively anonymous.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/pipe.hpp 
> a3574fd6f2ff1608396b47cad8cbed88134a74ca 
> 
> 
> Diff: https://reviews.apache.org/r/66957/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 66957: Windows: Enabled creating overlapped pipes with `os::pipe`.

2018-05-04 Thread Akash Gupta 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66957/
---

Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
John Kordich, Joseph Wu, and Radhika Jandhyala.


Bugs: MESOS-8674
https://issues.apache.org/jira/browse/MESOS-8674


Repository: mesos


Description
---

`os::pipe` was using `CreatePipe`, which does not support overlapped
io. Now, the implementation of `os::pipe` has been changed to use
`CreateNamedPipe`, which supports overlapped IO. The named pipe is
created with an unique random name, so it is effectively anonymous.


Diffs
-

  3rdparty/stout/include/stout/os/windows/pipe.hpp 
a3574fd6f2ff1608396b47cad8cbed88134a74ca 


Diff: https://reviews.apache.org/r/66957/diff/1/


Testing
---


Thanks,

Akash Gupta