----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27958/#review61449 -----------------------------------------------------------
3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27958/#comment103029> We should chain this with a 'then' so that you get discarding semantics for free. Let me elaborate. if you do: return io::poll(s, io::WRITE) .then(lambda::bind( &internal::connect, Socket(shared_from_this())); Then this lets someone discard the future that they get back which will cancel the poll and cause the resulting future to be discarded! Then you can just make 'connect' be: Future<Socket> connect(const Socket& socket) { ... if (getsockopt(s, SOL_SOCKET, SO_ERROR, &opt, &optlen) < 0 || opt != 0) { return Failure("Socket error while connecting"); } return socket; } This actually simplifies because now you don't even need the 'Connect' struct. Note that you could set up discarding yourself, but it's a lot more code so unnecessary. Make sense? 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27958/#comment103035> This is getting called for failures from internal::connect and getsockopt, which previous would not cause the process to abort via the PLOG(FATAL). Moreover, it looks like you're missing a socket_manager->close(s) call here? And does that race with the 'links[to].insert(process)' after the 'socket.connect(' chain below? I don't think this should PLOG(FATAL), I don't think the original code should have done that either, but we definitely need to socket_manager->close(s). Also, any reason not to print out the error message? We shoud at least send it to VLOG(1). Note for the future though, if you don't care about the argument you can just leave off the lambda::_1 in the bind and then not need to have an unnamed parameter. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27958/#comment103037> I don't see receiving_connect deleted in this patch, haven't we replaced it? 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27958/#comment103042> Same comments as above re: socket_manager->close(s) and not aborting. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27958/#comment103041> Same comment as above re: removing sending_connect. - Benjamin Hindman On Nov. 13, 2014, 2:52 a.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27958/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2014, 2:52 a.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-1330 > https://issues.apache.org/jira/browse/MESOS-1330 > > > Repository: mesos-git > > > Description > ------- > > See Summary. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > 66838814236fc064a2463399fb15f4a815879bf5 > 3rdparty/libprocess/src/process.cpp > a34b8702b01dec9c954552de0b923866d172c453 > > Diff: https://reviews.apache.org/r/27958/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joris Van Remoortere > >
