> On 2011-09-27 13:59:22, Camille Fournier wrote: > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java, line 195 > > <https://reviews.apache.org/r/1714/diff/1/?file=37950#file37950line195> > > > > Why was this removed? It is not germane to the patch at hand. > > Thomas Koch wrote: > You're right, the removal had nothing to do with the issue, but it was > the right thing to do, once I've been there. It may happen in rare cases, > that connect returns true already there, but primeConnection() is called > anyways later from doTransport(). Having to possible call origins only adds > to uncertainty. > Did you observe any problems with this? > > Camille Fournier wrote: > Are you sure that the lines in doTransport() will be called and executed > if the socket connects immediately? I don't personally know whether that is > the case or not. You didn't write any tests to show that this would happen, > and you shouldn't have made the change for purposes of removing uncertainty > without explicitly calling out what you were doing. If you are quite sure > that even in the case of an immediate connection, OP_CONNECT will be set and > we will hit that in doTransport, I'm fine with this change. But I don't like > having changes like this snuck in under the radar, because if this is not the > case and it causes a bug, it will be a massive pain to find.
It seems to me, by the by, that if a socket is immediately connected it will NOT set the selection key for OP_CONNECT, because state will be set to ST_CONNECTED (see the source for SocketChannelImpl, specifically connect: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.connect%28java.net.SocketAddress%29 and translateReadyOps: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/nio/ch/SocketChannelImpl.java#SocketChannelImpl.translateReadyOps%28int%2Cint%2Csun.nio.ch.SelectionKeyImpl%29 So, I'm not sure that this is a safe change to make, and you should always, always, always, be careful when changing anything to do with our NIO code unless you are really sure of the consequences. - Camille ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1714/#review2090 ----------------------------------------------------------- On 2011-09-14 22:18:24, Thomas Koch wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1714/ > ----------------------------------------------------------- > > (Updated 2011-09-14 22:18:24) > > > Review request for zookeeper. > > > Summary > ------- > > . > > > This addresses bug ZOOKEEPER-786. > https://issues.apache.org/jira/browse/ZOOKEEPER-786 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java 626da04 > > Diff: https://reviews.apache.org/r/1714/diff > > > Testing > ------- > > > Thanks, > > Thomas > >
