----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31930/#review76268 -----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment123822> 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... 3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment123787> 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) 3rdparty/libprocess/include/process/http.hpp <https://reviews.apache.org/r/31930/#comment123818> 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? 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123791> 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?" 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123789> typo: 'to' -> 'so' 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123790> I think just a std::swap(data->promises, promises); will make it more clear what is happening here, and be more concise. 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123792> 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.' 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123797> Regarding the multiple-dispatch usage pattern of 'read()' above: If 2 writes are happening simultaneously, and the context switch point is after the lock release (line 147), then the promises will get satisfied in a different order than they were pulled from the promises queue (i.e. registered by 'read()' calls). F1 = read(); F2 = read(); F2 satisfied. F1 satisfied. 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123795> Regarding the multiple-dispatch usage pattern of 'read()' above: If a Reader::close() happens at L147, between this critical section, and the promise->set(s), then in the code that handles the Reader's futures this might happen: F1: data F3: closed F2: data 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123798> same typo: 'to' -> 'so' 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123799> use std::swap() as above. 3rdparty/libprocess/src/http.cpp <https://reviews.apache.org/r/31930/#comment123800> Just a reminder to convert these to None() to signify EOF. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/31930/#comment123820> Can you leave a comment as to why you took a copy before calling 'close()'? (To remove constness). Here and below. 3rdparty/libprocess/src/tests/http_tests.cpp <https://reviews.apache.org/r/31930/#comment123832> 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. - Joris Van Remoortere 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 > >
