----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67389/#review204501 -----------------------------------------------------------
3rdparty/libprocess/src/libwinio_impl.cpp Lines 53-59 (patched) <https://reviews.apache.org/r/67389/#comment286979> Should we `s/CALLBACK/__stdcall` so that this is _slightly_ more clear for non-Windows readers (and maybe add a brief comment explaining why this is necessary for Windows, e.g. that the Win32 APIs use a different calling convention than C++). Also, we definitely should add a comment explaining that this is a TimerAPCProc callback function, and is used for SetWaitableTimer (with links to https://msdn.microsoft.com/en-us/library/windows/desktop/ms686786(v=vs.85).aspx and https://msdn.microsoft.com/en-us/library/windows/desktop/ms686289(v=vs.85).aspx). 3rdparty/libprocess/src/libwinio_impl.cpp Lines 90 (patched) <https://reviews.apache.org/r/67389/#comment286980> s/io/IO 3rdparty/libprocess/src/libwinio_impl.cpp Lines 91-119 (patched) <https://reviews.apache.org/r/67389/#comment286981> Would this be cleaner with a templated struct? ``` template <typename T> struct OverlappedIO { Promise<T>* promise; OVERLAPPED overlapped; HANDLE handle; }; struct OverlappedReadWrite : OverlappedIO<size_t> {}; struct OverlappedAccept : OverlappedIO<Nothing> { unsigned char buf[sizeof(SOCKADDR_STORAGE) + 16]; }; ``` Since they _all_ hold a `Promise<T> promise`. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 130 (patched) <https://reviews.apache.org/r/67389/#comment286982> I'd double check that `stringify(dword)` gets the error message; I think you probably want `+ Error(error).message` 3rdparty/libprocess/src/libwinio_impl.cpp Lines 137-138 (patched) <https://reviews.apache.org/r/67389/#comment286986> Wait, how? `OVERLAPPED` is a Windows type, and `IOOverlappedBase` is a type we defined... which has an `OVERLAPPED` field. I'm either missing something here or we're doing something weird... 3rdparty/libprocess/src/libwinio_impl.cpp Lines 161-170 (patched) <https://reviews.apache.org/r/67389/#comment286984> ``` std::unique_ptr<Promise<size_t>> promise(io_read.promise); ``` Default deleter uses `delete` operator, so then you'd have free RAII and no `delete promise;` in each of these cases. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 171 (patched) <https://reviews.apache.org/r/67389/#comment286985> Since we do nothing after the switch, opt for `return` over `break` to lessen cognitive overhead. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 219-220 (patched) <https://reviews.apache.org/r/67389/#comment286987> Could we just `return false`? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 233 (patched) <https://reviews.apache.org/r/67389/#comment286988> Could we just `return true`? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 244 (patched) <https://reviews.apache.org/r/67389/#comment286989> Could we just `return true`? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 247 (patched) <https://reviews.apache.org/r/67389/#comment286990> And then should this be `UNREACHABLE();` or `true`? That is, can `entry->lpCompletionKey` be a value not covered in the switch? Current code leaves this as an open question... 3rdparty/libprocess/src/libwinio_impl.cpp Lines 286 (patched) <https://reviews.apache.org/r/67389/#comment286991> Should we really wait infinitely? Surely there should be a timeout at some point. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 325 (patched) <https://reviews.apache.org/r/67389/#comment286992> Is there ever a case where, at this point, `loop == false`? I'm wondering why it's `loop = loop && continue_loop` instead of `loop = continue_loop`. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 348 (patched) <https://reviews.apache.org/r/67389/#comment286993> s/unamed/unnamed 3rdparty/libprocess/src/libwinio_impl.cpp Lines 349-350 (patched) <https://reviews.apache.org/r/67389/#comment286994> Eh, even for Win32 APIs, use `nullptr`. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 354-356 (patched) <https://reviews.apache.org/r/67389/#comment286995> LOL 3rdparty/libprocess/src/libwinio_impl.cpp Lines 386 (patched) <https://reviews.apache.org/r/67389/#comment286996> Everywhere else we were using `HANDLE`; and here we switched to `int_fd`; I'm guessing this is a more "public" API? (Scrolls way up...) Yup. Okay, maybe a comment :D 3rdparty/libprocess/src/libwinio_impl.cpp Lines 428 (patched) <https://reviews.apache.org/r/67389/#comment286997> use or capture? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 436 (patched) <https://reviews.apache.org/r/67389/#comment286998> I don't think you need to explicitly cast it, but you can leave it if you think it's more clear. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 457-462 (patched) <https://reviews.apache.org/r/67389/#comment286999> Can this just use the arguments to `make_shared`, e.g.: ``` auto o = std::make_shared<OverlappedReadWrite>(new Promise<size_t>(), OVERLAPPED{}, fd, type); ``` 3rdparty/libprocess/src/libwinio_impl.cpp Lines 467-468 (patched) <https://reviews.apache.org/r/67389/#comment287001> Should we do any sort of `CHECK` on this `void* buf`, or is that taken care of elsewhere? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 468 (patched) <https://reviews.apache.org/r/67389/#comment287000> Why is the `reinterpret_cast` necessary? `&overlapped->overlapped` should just be an `OVERLAPPED*`... 3rdparty/libprocess/src/libwinio_impl.cpp Lines 478-483 (patched) <https://reviews.apache.org/r/67389/#comment287002> Why do we care about the promise at this point, since we're about to delete it, and if this was immediate, then nothing else ever obtained the promise? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 572-574 (patched) <https://reviews.apache.org/r/67389/#comment287004> Maybe document that `overlapped->buf` will not be used to store received data (first argument of 0), but will be used to store two addresses, local (in the first half), and remote (in the second half). You know how much I hate reading MSDN ;) 3rdparty/libprocess/src/libwinio_impl.cpp Lines 576 (patched) <https://reviews.apache.org/r/67389/#comment287005> Okay, here's that reinterpret cast from our `OVERLAPPED` wrapper struct to Windows' `OVERLAPPED` struct again; what's going on? 3rdparty/libprocess/src/libwinio_impl.cpp Lines 598-627 (patched) <https://reviews.apache.org/r/67389/#comment287008> What's this do exactly? Pretend I know nothing. Based on some searching... I guess that the ConnectEx function we need to call is not something that's exported from a DLL, but is expected to be obtained via a function pointer at runtime. Definitely some Windows hackery going on here that we should document... 3rdparty/libprocess/src/libwinio_impl.cpp Lines 600 (patched) <https://reviews.apache.org/r/67389/#comment287006> Normally I shun using the `LP` Hungarian notation... but the underlying type in Windows is atrocious... 3rdparty/libprocess/src/libwinio_impl.cpp Lines 624 (patched) <https://reviews.apache.org/r/67389/#comment287007> ;) 3rdparty/libprocess/src/libwinio_impl.cpp Lines 654-657 (patched) <https://reviews.apache.org/r/67389/#comment287009> Ah ha. Per the note under the remarks section in the ConnectEx MSDN docs. 3rdparty/libprocess/src/libwinio_impl.cpp Lines 718 (patched) <https://reviews.apache.org/r/67389/#comment287010> ;) 3rdparty/libprocess/src/libwinio_impl.cpp Lines 745 (patched) <https://reviews.apache.org/r/67389/#comment287011> I made it! - Andrew Schwartzmeyer On May 30, 2018, 11:54 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67389/ > ----------------------------------------------------------- > > (Updated May 30, 2018, 11:54 a.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, > John Kordich, Joseph Wu, and Radhika Jandhyala. > > > Bugs: MESOS-8668, MESOS-8671 and MESOS-8672 > https://issues.apache.org/jira/browse/MESOS-8668 > https://issues.apache.org/jira/browse/MESOS-8671 > https://issues.apache.org/jira/browse/MESOS-8672 > > > Repository: mesos > > > Description > ------- > > The Windows IOCP async backend, called libwinio, provides a single > threaded event loop implementation that allows for async IO and timer > events. libwinio wraps the native Win32 async functions using > libprocess's primitives, which makes it easier to use and more type > safe. > > > Diffs > ----- > > 3rdparty/libprocess/src/CMakeLists.txt > cf443dffd0663ecf02b7efd6f7094175b94aae19 > 3rdparty/libprocess/src/libwinio_impl.hpp PRE-CREATION > 3rdparty/libprocess/src/libwinio_impl.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/67389/diff/1/ > > > Testing > ------- > > > Thanks, > > Akash Gupta > >
