----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53803/#review156677 -----------------------------------------------------------
Thanks for taking on this issue and testing it! 3rdparty/libprocess/src/tests/http_tests.cpp (lines 203 - 205) <https://reviews.apache.org/r/53803/#comment226876> This should be a SocketTest.ReceiveEOF within socket_tests.cpp? There doesn't appear to be any HTTP involved here and we're just interested in the socket behavior. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 203 - 204) <https://reviews.apache.org/r/53803/#comment226898> A lot of these comments seem to have a really long line followed by a really short line. I would suggest trying to wrap these comments to be a little less "jagged", as I find it's easier on the eyes, but up to you: ``` // This test verifies that an EOF sent on a socket will be correctly received by // the other end. ``` vs. ``` // This test verifies that an EOF sent on a socket // will be correctly received by the other end. ``` Ditto below. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 210 - 212) <https://reviews.apache.org/r/53803/#comment226896> Is it possible to just use the try directly? ``` Try<Socket> client = Socket::create(); ASSERT_SOME(client); ... client->recv(); ``` Ditto below for the server socket: ``` Try<Socket> server = Socket::create(); ASSERT_SOME(server); server->bind(Address()); ``` Ditto for the Address, can you use it via the Try instead of pulling out another variable? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 218 - 219) <https://reviews.apache.org/r/53803/#comment226877> Can you use explicit shutdowns instead of relying on the scoping? 3rdparty/libprocess/src/tests/http_tests.cpp (line 225) <https://reviews.apache.org/r/53803/#comment226897> Using the default constructor seems a little less clear than: ``` server.bind(Address::LOCALHOST_ANY()) ``` Also can you check the result? And could you just use the result to avoid the need to call server.address()? ``` Try<Address> server_address = server.bind(Address::LOCALHOST_ANY()); ASSERT_SOME(server_address); ... // use it: server_address.get(); ``` 3rdparty/libprocess/src/tests/http_tests.cpp (lines 237 - 240) <https://reviews.apache.org/r/53803/#comment226889> Can this just be: ``` AWAIT_READY(server_socket.send(data)); AWAIT_EXPECT_EQ(data, client_socket.recv(data.size())); ``` Or is there some reason you wanted to call recv first? If so, a comment would be great. Note that you don't need the '`receive`' variable to be in the outer scope. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 243 - 248) <https://reviews.apache.org/r/53803/#comment226878> Oh, can you just put your shutdown code before this patch? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 250 - 252) <https://reviews.apache.org/r/53803/#comment226886> Can this just be: ``` AWAIT_EXPECT_EQ("", client_socket.recv()); ``` i.e. why do you need to pass 1? and doesn't look like you need to store the future in a variable? - Benjamin Mahler On Nov. 16, 2016, 7:20 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53803/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2016, 7:20 p.m.) > > > Review request for mesos, Benjamin Mahler and Joseph Wu. > > > Bugs: MESOS-5966 > https://issues.apache.org/jira/browse/MESOS-5966 > > > Repository: mesos > > > Description > ------- > > This patch adds HTTPTest.SocketEOF to verify that EOFs are > reliably received whether or not there is a pending recv() > request at the time the EOF is received. > > > Diffs > ----- > > 3rdparty/libprocess/src/tests/http_tests.cpp > 533104c93dd1eaf67bf3752163d2e0cad090078d > > Diff: https://reviews.apache.org/r/53803/diff/ > > > Testing > ------- > > Testing details are at the end of this review chain. > > > Thanks, > > Greg Mann > >
