> On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/libprocess/src/libwinio_impl.cpp > > Lines 91-119 (patched) > > <https://reviews.apache.org/r/67389/diff/1/?file=2032283#file2032283line91> > > > > 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`. > > Akash Gupta wrote: > Yeha that could work. You still need the IOType to determine what > Overlapped object you actually got in the IOCP loop since that is only > determined at runtime. > > Akash Gupta wrote: > Eh I realized why I didn't used templates. Since you need to use the > IOType to determine what overlapped object actually got returned from > GetQueuedCompletionStatus, you need to cast it to a some "base" type that has > the IOType field. Even with templates, you need to do this, so there wasn't > much benefit to it.
Aw okay. > On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/libprocess/src/libwinio_impl.cpp > > Lines 137-138 (patched) > > <https://reviews.apache.org/r/67389/diff/1/?file=2032283#file2032283line137> > > > > 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... > > Akash Gupta wrote: > It's like C style "polymorphism". The Windows async functions only care > about the OVERLAPPED object, so you can put anything after the struct like > this: > ``` > struct X { > OVERLAPPED overlapped; > <user data> > }; > ``` > > Then you can reinterpret_cast `X*` to `OVERLAPPED*` when sending it to > the Win32 functions and then cast same `OVERLAPPED*` pointer received from > the IOCP loop to `X*` again when you are in your IOCP callbacks. Yes, this is > super type unsafe, but IOCP is type unsafe in general and this way is > basically expected style: > https://blogs.msdn.microsoft.com/oldnewthing/20101217-00/?p=11983 > > Another (type unsafe) way is to pass the `&X->overlapped` address to the > Win32 functions and then use the `CONTAINING_RECORD` macro to determine the > address of the parent structure like this: > ``` > #define OFFSET_OF(T, m) ((size_t) &(((T*) 0)->m))) // Gets offset of > field m in struct T. > #define CONTAINERING_RECORD(p, T, m) (T*) ((char*) p - OFFSET_OF(T, m)) > // Get the address of parent struct of given a pointer one of the child > members (p = &T->m). > > > IOOverlappedBase* overlapped = CONTAINERING_RECORD(overlapped_, > IOOverlappedBase, overlapped_field); > ``` Gotcha. I hate C... add some extra comments, at least at the point this is first introduced :) > On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/libprocess/src/libwinio_impl.cpp > > Lines 286 (patched) > > <https://reviews.apache.org/r/67389/diff/1/?file=2032283#file2032283line286> > > > > Should we really wait infinitely? Surely there should be a timeout at > > some point. > > Akash Gupta wrote: > This is the event loop that handles all Mesos IO and timer events, so > what would we do when we time out? Exit? I guess nothing, it just feels weird. > On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote: > > 3rdparty/libprocess/src/libwinio_impl.cpp > > Lines 467-468 (patched) > > <https://reviews.apache.org/r/67389/diff/1/?file=2032283#file2032283line467> > > > > Should we do any sort of `CHECK` on this `void* buf`, or is that taken > > care of elsewhere? > > Akash Gupta wrote: > I'm not sure what you mean here. What we would `CHECK` `void* buf` for? > The size is `CHECK'd` in `os::read_async`. Sorry, I guess my question (from this snippet of code) is who allocates and owns the memory under `void* buf`? I'm assuming the caller allocated it before sending it to us, we're just hoping here that was done correctly. For instance, if we're given `nullptr` we might explode. I need to go look at where this is called... - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67389/#review204501 ----------------------------------------------------------- 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 > >
