KAFKA-6300 SelectorTest may fail with ConcurrentModificationException

[tedyu]

Synchronization is added w.r.t. sockets ArrayList to avoid ConcurrentModificationException

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[ijuma]

cc @rajinisivaram

[rajinisivaram]

@tedyu Thanks for the PR.

SslSelectorTest.testImmediatelyConnectedCleaned fails intermittently with ConcurrentModificationException in the tearDownbecause it creates client connections and goes straight into tearDown and closes the server. Since the connections may not have been processed by the server yet, it can still add to the sockets list while the server is iterating through the list to close all sockets. Synchronizing on sockets would avoid ConcurrentModificationException , but it can potentially leak sockets and threads. We need to ensure that once EchoServer#close` is called, no more sockets are added to the list. Any socket processed after that should be closed immediately and the thread to process the sockets should not be created.

[viktorsomogyi]

Hi Ted, I quickly had a look at this change if you don't mind. My concern is the following:
Let's suppose that if right after serverSocket.accept() a closeConnection() gets scheduled and completes, then this block would indeed run without adding the current socket to a list but it would still start the thread with a leaked socket.
I think we shouldn't even accept any sockets or start threads if the server is being shut down.
Let me know if you disagree or I missed something :).

[tedyu]

@viktorsomogyi
I agree.
I was about to make this change.

[tedyu]

retest this please

[rajinisivaram]

closing flag should be checked inside the synchronized block. Otherwise, this block could be executed after closeConnections on a socket that was already accepted, but not added to the list (this is the cause of the transient test failure). The !closing check in while is not really needed since accept would just throw IOException and return, but you want to keep it, it should be synchronized or made volatile.

[tedyu]

Once closing flag becomes true, the while loop in EchoServer#run() should exit.
What about the last assertion in testServerDisconnect() ?

        assertEquals("hello", blockingRequest(node, "hello"));

It seems the above should be dropped - otherwise it would hang.

[rajinisivaram]

@ tedyu left comments on fixing this.

[rajinisivaram]

closing should be set in close() rather than closeConnections - i.e. set in when closing server socket, not when closing client connections.

[rajinisivaram]

Can you revert this change please, after setting closing flag in EchoServer#close()?

[rajinisivaram]

@tedyu Can you also rebase the PR since it has conflicts? Thanks.

[tedyu]

New PR coming which addresses Rajini's comments.