> On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > >
Great comments, thank you Joris! > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/process.cpp, line 202 > > <https://reviews.apache.org/r/31930/diff/1/?file=891211#file891211line202> > > > > Can you leave a comment as to why you took a copy before calling > > 'close()'? (To remove constness). > > Here and below. This is a very common pattern (e.g. discarding a const future), but it seems nice to have for those unfamiliar. Will add it! > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/include/process/http.hpp, line 168 > > <https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line168> > > > > Can we augment the return type to represent the EOF state? For example > > we can return Future<Option<std::string>> where None() represents EOF. > > > > Consider the case where the user of the API writes an empty string. > > This would trigger an EOF, which may not be intentional. > > > > This will help if we want to maintain the 1-1 mapping of 'read()' > > future callback invocations to 'write()' calls, and remove the empty string > > checks. > > If we don't want to maintain this relationship, why not? If not, we > > would have to propogate to all users of the API that there will not be a > > 'read()' call corresponding to that empty string 'write()' call. This seems > > brittle. > > > > The users of this API will not be surprised when their 'read()' doesn't > > trigger upon an empty 'write()'. > > > > If we also want to be able to encapsulate errors, we could use a Result? The 1-1 mapping is not considered part of this API (rather just an implementation detail, and only for non-empty writes!). I've updated the comment on top to avoid getting tripped up by this. Being more explicit about EOF sounds good to me. It is a bit of a departure from calling `read()` on a pipe (which returns empty read when end of file is reached), and it's a bit more complicated for callers (need to deal with empty string, which is a special case for things like chunked encoding), but I agree it's nice to capture very explicitly that we've reached the end. > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/http.cpp, lines 117-119 > > <https://reviews.apache.org/r/31930/diff/1/?file=891210#file891210line117> > > > > I think if we move this up into the 'closed' predicate check, then it > > is clearer to the reader that we only notify upon the 1st close call.' Sounds good. > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/http.cpp, lines 96-99 > > <https://reviews.apache.org/r/31930/diff/1/?file=891210#file891210line96> > > > > I think just a std::swap(data->promises, promises); will make it more > > clear what is happening here, and be more concise. Hm.. not bad.. not great though. It seems less explicit because it's only important that 'data->promises' is transfered over to 'promises', it's not important that 'promises' is transferred to 'data->promises', right? With `std::swap` you need to understand that 'promises' is empty, right? :) I'll go with `std::swap` though since it does seem cleaner and more efficient, despite this concern. > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/http.cpp, lines 90-99 > > <https://reviews.apache.org/r/31930/diff/1/?file=891210#file891210line90> > > > > Is it really possible that both these queues contain data at the same > > time? If not, can we make this clear with a CHECK? It would make it easier > > to understand what is going on. > > > > Without this information, one could ask "Why are we destroying data > > that should have just been sent to the pending read requests?" No, only one of these can be non-empty. I've ditched the terminology borrowed from process::Queue and renamed the queues to 'reads' and 'writes', hopefully it's a lot clearer when reading the code now. Once a reader closes it's end, it cannot read the outstanding data, so it must be thrown away. Is there something I'm missing? Why would one ask that question? > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/src/tests/http_tests.cpp, line 177 > > <https://reviews.apache.org/r/31930/diff/1/?file=891212#file891212line177> > > > > Can we add a test that covers the copying behavior of Reader / Writer? > > (regarding the high-level usage pattern mentioned above). > > > > For example we should not be able to read from a copied reader if one > > of the instances has closed. Will punt on that for now since we're not planning to use concurrent Readers / Writers due to the lack of ordering, whereas we're expecting our HTTP bodies to be ordered. > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 165-168 > > <https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line165> > > > > Can we add a comment about the 2 usage patterns for 'read()': > > 1) Call 'read()' one at a time. This guarantees order of data vs EOF vs > > closed. > > 2) Make multiple 'read()' calls while waiting on multiple futures. This > > has no order guarantees due to the races between the locked and callback > > invocation sections. (see comments below) Great eye Joris! I also noticed this when initially making this change as it's a property of process::Queue, see the note I left here: https://reviews.apache.org/r/31788/diff/1/#1 However, after having chatted with Jie at the time, and having thought through this further, this is expected for a few reasons: * Our current callback model on Future is to invoke callbacks synchronously, however we should not be allowing callers to assume this is the case! For example, we even had a TODO [here](https://github.com/apache/mesos/blob/0.21.0/3rdparty/libprocess/include/process/future.hpp#L1591) to invoke callbacks from another execution context. Unfortunately that TODO was wiped out in your change [here](https://reviews.apache.org/r/28195) :) I just restored that TODO to make it clear that we don't want to be guaranteeing synchronous callback execution. * Even if we were setting these in the critical section, the callers may not have added the callbacks on the returned Futures yet! (if they context switch after calling read but before calling e.g. `.onAny()`) This means we still would not be maintaining ordering of the callback invocation, since we do not control when the callbacks are added, the callers do. I think we want to avoid specifying this kind of behavior on http::Pipe, since it's inherent to libprocess and our asychronous model. Also, we're not planning to use http::Pipe concurrently like this. > On March 13, 2015, 1:25 a.m., Joris Van Remoortere wrote: > > 3rdparty/libprocess/include/process/http.hpp, lines 129-145 > > <https://reviews.apache.org/r/31930/diff/1/?file=891209#file891209line129> > > > > Can we add a high-level comment about the intended usage pattern? > > > > -We can have multiple readers mapped to the same pipe, and the data is > > shared between them. Once one reader calls 'close()', all the readers' > > futures are completed with EOFs... > Once one reader calls 'close()', all the readers' futures are completed with > EOFs... Do you mean when the writer calls close and all data has been read? That's when the reads return EOF, per the comment above ("much like unix pipes ..."). Per our discussion below, I'd like to avoid documenting ineherent concurrency properties of libprocess here. We're planning to only use this with one reader and one writer, much like process::Queue. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76268 ----------------------------------------------------------- On March 11, 2015, 7:41 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31930/ > ----------------------------------------------------------- > > (Updated March 11, 2015, 7:41 a.m.) > > > Review request for mesos, Benjamin Hindman and Jie Yu. > > > Bugs: MESOS-2438 > https://issues.apache.org/jira/browse/MESOS-2438 > > > Repository: mesos > > > Description > ------- > > See MESOS-2438 for the motivation. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/http.hpp > 10143fdbd5eb43f968c41957a2335f96a097d82e > 3rdparty/libprocess/src/http.cpp 7c0cee404645e1dd1a9253f85883aefa29a5f999 > 3rdparty/libprocess/src/process.cpp > e7b029ba97e640c2102548c190ba62b30602f43d > 3rdparty/libprocess/src/tests/http_tests.cpp > 800752a57230d7768d3d741fef87f6283757e424 > > Diff: https://reviews.apache.org/r/31930/diff/ > > > Testing > ------- > > * added tests > * make check > > > Thanks, > > Ben Mahler > >
