You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2018/05/30 18:58:11 UTC
Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Bugs: MESOS-5371 and MESOS-8668
https://issues.apache.org/jira/browse/MESOS-5371
https://issues.apache.org/jira/browse/MESOS-8668
Repository: mesos
Description
-------
Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
so we need to wrap the function around our own code to make it so. We
need to keep it inside the `WindowsFD` class, because there isn't a way
to determine if a HANDLE is associated with an IOCP through the Win32
API.
Diffs
-----
3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
3rdparty/stout/include/stout/os/windows/fd.hpp 5dbdff2680370d123579c5e3fdd9b0e0adaf512e
Diff: https://reviews.apache.org/r/67385/diff/1/
Testing
-------
Thanks,
Akash Gupta
Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/#review205308
-----------------------------------------------------------
Ship it!
Two more typos for Andy to fix before committing.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 203 (patched)
<https://reviews.apache.org/r/67385/#comment288241>
Nit: s/bookeeping/bookkeeping/
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 215 (patched)
<https://reviews.apache.org/r/67385/#comment288240>
Nit: s/assigns/assign/
- Joseph Wu
On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:58 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
> 3rdparty/stout/include/stout/os/windows/fd.hpp 89a037af31ad68a275d2519afbe4f161b23efe91
>
>
> Diff: https://reviews.apache.org/r/67385/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/#review205218
-----------------------------------------------------------
Ship it!
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 245 (patched)
<https://reviews.apache.org/r/67385/#comment288162>
Nit that I'll fix when commiting: missing a space ;)
- Andrew Schwartzmeyer
On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:58 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
> 3rdparty/stout/include/stout/os/windows/fd.hpp 89a037af31ad68a275d2519afbe4f161b23efe91
>
>
> Diff: https://reviews.apache.org/r/67385/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On June 5, 2018, 2:02 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/os/windows/fd.hpp
> > Lines 183-184 (patched)
> > <https://reviews.apache.org/r/67385/diff/1/?file=2032269#file2032269line185>
> >
> > Is there ever a case where `iocp_handle_` hasn't been allocated? Right now, this is implicitly relying on the fact that we chose to make the default constructor construct off the `HANDLE` ctor with `INVALID_HANDLE_VALUE`, and that ctor uses `make_shared`. Just wondering if we should refactor to make the allocation more explicit and certain (like maybe to `make_shared` in the default ctor, and ensure all ctors call it too, I don't know yet).
>
> Akash Gupta wrote:
> THere isn't a case, although an allocated `iocp_handle_` is only needed for the "valid" `HANDLE/SOCKET` constructors. I think it makes sense to have the most general (most params) constructor call `make_shared` and the narrower constructors call the more general one like how it works now.
That makes sense.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/#review204345
-----------------------------------------------------------
On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:58 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
> 3rdparty/stout/include/stout/os/windows/fd.hpp 5dbdff2680370d123579c5e3fdd9b0e0adaf512e
>
>
> Diff: https://reviews.apache.org/r/67385/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/#review204345
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181 (patched)
<https://reviews.apache.org/r/67385/#comment286813>
Maybe just `HANDLE handle` ;)
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181-182 (patched)
<https://reviews.apache.org/r/67385/#comment286819>
What is the `key` used for? (Maybe we need a small excerpt from the MSDN docs for `CreateIoCompletionPort`.)
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 183 (patched)
<https://reviews.apache.org/r/67385/#comment286812>
I'm not saying you should use it, or if you even want to use it, but there does exist some synchronization helpers in stout, under `synchronized.hpp`.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 183-184 (patched)
<https://reviews.apache.org/r/67385/#comment286817>
Is there ever a case where `iocp_handle_` hasn't been allocated? Right now, this is implicitly relying on the fact that we chose to make the default constructor construct off the `HANDLE` ctor with `INVALID_HANDLE_VALUE`, and that ctor uses `make_shared`. Just wondering if we should refactor to make the allocation more explicit and certain (like maybe to `make_shared` in the default ctor, and ensure all ctors call it too, I don't know yet).
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 184 (patched)
<https://reviews.apache.org/r/67385/#comment286820>
`const`
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 188 (patched)
<https://reviews.apache.org/r/67385/#comment286818>
Oi, I hate that our cast operators are implicit, `*this` threw me for a minute.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 202-215 (patched)
<https://reviews.apache.org/r/67385/#comment286809>
Old comment, but leaving for posterity:
I don't like this `WindowsFD::dup` method; it opens up a can of worms, such as: should we move the `os::dup` logic into this method (should we then start moving other `os::foo(int_fd)` logic into their own methods)? It also prompted me to think about this maybe being done in a copy ctor, but that doesn't make sense either.
At a minimum, let's make this `private` and make `os::dup` a `friend` so that `WindowsFD::dup` is clearly demarcated as an internal helper.
And once I wrote that, I almost want to suggest: make `os::dup` a friend, and just make a new `fd` and manually assign the `overlapped_` and `iocp_handle_` fields (that is, get rid of the helper abstraction). Not sure if that would be cleaner.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 231-235 (patched)
<https://reviews.apache.org/r/67385/#comment286815>
This is more like `struct IOCPData`, since it's both the handle and the mutex.
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 237 (patched)
<https://reviews.apache.org/r/67385/#comment286814>
This could just be named `iocp_`
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 239-257 (patched)
<https://reviews.apache.org/r/67385/#comment286811>
As discussed in-person, we can simplify all this logic (read: delete these ctors and the `dup` method) by making `os::dup` a friend, using the default copy ctor in `os::dup`, and then overwriting the `handle_` or `socket_` field with the duplicated object (which is then doable as a friend function).
- Andrew Schwartzmeyer
On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:58 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
> 3rdparty/stout/include/stout/os/windows/fd.hpp 5dbdff2680370d123579c5e3fdd9b0e0adaf512e
>
>
> Diff: https://reviews.apache.org/r/67385/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67385: Windows: Added IOCP `HANDLE` to `WindowsFD`.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67385/#review204284
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/fd.hpp
Lines 181 (patched)
<https://reviews.apache.org/r/67385/#comment286697>
Suggestion: It's easy to get `iocp_handle` and `iocp_handle_` mixed up while reading this function. Consider renaming the parameter `iocp_handle` to `target`.
- Joseph Wu
On May 30, 2018, 11:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67385/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:58 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-5371 and MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-5371
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos/Libprocess uses `os::nonblock()` in a way that assumes that it is
> idempotent. Unfortunately, `CreateIoCompletionPort` is not idempotent,
> so we need to wrap the function around our own code to make it so. We
> need to keep it inside the `WindowsFD` class, because there isn't a way
> to determine if a HANDLE is associated with an IOCP through the Win32
> API.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/dup.hpp 5bda095e676b038cdaea04f7be23ba2a1aca9015
> 3rdparty/stout/include/stout/os/windows/fd.hpp 5dbdff2680370d123579c5e3fdd9b0e0adaf512e
>
>
> Diff: https://reviews.apache.org/r/67385/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>