kezhuw commented on code in PR #2047:
URL: https://github.com/apache/zookeeper/pull/2047#discussion_r1330402613
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNIO.java:
##########
@@ -256,8 +256,8 @@ SocketChannel createSock() throws IOException {
* @throws IOException
*/
void registerAndConnect(SocketChannel sock, InetSocketAddress addr) throws
IOException {
- sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
boolean immediateConnect = sock.connect(addr);
+ sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
Review Comment:
Asides from above, I think we should skip register in case of
`immediateConnect` otherwise we may prime the connection twice. But it is
probably another story and need verification.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNIO.java:
##########
@@ -256,8 +256,8 @@ SocketChannel createSock() throws IOException {
* @throws IOException
*/
void registerAndConnect(SocketChannel sock, InetSocketAddress addr) throws
IOException {
- sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
boolean immediateConnect = sock.connect(addr);
+ sockKey = sock.register(selector, SelectionKey.OP_CONNECT);
Review Comment:
> but because in ClientCnxnSocketNIO::registerAndConnect method socket is
registed to selector firstly and do sock.connect operation leading the fd of
sock can't be closed.
https://github.com/openjdk/jdk/blob/jdk8-b120/jdk/src/share/classes/sun/nio/ch/SocketChannelImpl.java#L841
```java
// If this channel is not registered then it's safe to close the
fd
// immediately since we know at this point that no thread is
// blocked in an I/O operation upon the channel and, since the
// channel is marked closed, no thread will start another such
// operation. If this channel is registered then we don't close
// the fd since it might be in use by a selector. In that case
// closing this channel caused its keys to be cancelled, so the
// last selector to deregister a key for this channel will invoke
// kill() to close the fd.
//
if (!isRegistered())
kill();
```
`sock.close` will not close fd due to registered status. It is sad that:
* We never got a chance to `ClientCnxnSocketNIO#doTransport` which call
`Selector::select` which process cancelled key and close fd.
* We don't have client side session expiration, so never a chance to
`ClientCnxnSocketNIO#close` and `Selector::close`. #2058
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
