Skip to content

Close TcpTransport on RST in some Spots to Prevent Leaking TIME_WAIT Sockets #26764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
original-brownbear merged 3 commits into from
Sep 26, 2017
Merged

Close TcpTransport on RST in some Spots to Prevent Leaking TIME_WAIT Sockets #26764

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
feb8f30
#26701 retry in org.elasticsearch.transport.MockTcpTransport#connect…
original-brownbear Sep 23, 2017
f79f008
#27071 Stop lingering on closed client connections in MockTcpTransport
original-brownbear Sep 25, 2017
1ff8d6e
#26701 Added option to RST instead of FIN to TcpTransport#closeChannels
original-brownbear Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,12 @@ protected InetSocketAddress getLocalAddress(MockChannel mockChannel) {
@Override
protected MockChannel bind(final String name, InetSocketAddress address) throws IOException {
MockServerSocket socket = new MockServerSocket();
socket.bind(address);
socket.setReuseAddress(TCP_REUSE_ADDRESS.get(settings));
ByteSizeValue tcpReceiveBufferSize = TCP_RECEIVE_BUFFER_SIZE.get(settings);
if (tcpReceiveBufferSize.getBytes() > 0) {
socket.setReceiveBufferSize(tcpReceiveBufferSize.bytesAsInt());
}
socket.bind(address);
Copy link
Contributor

@s1monw s1monw Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense ++

MockChannel serverMockChannel = new MockChannel(socket, name);
CountDownLatch started = new CountDownLatch(1);
executor.execute(new AbstractRunnable() {
Expand Down Expand Up @@ -303,6 +303,7 @@ public void accept(Executor executor) throws IOException {
MockChannel incomingChannel = null;
try {
configureSocket(incomingSocket);
incomingSocket.setSoLinger(true, 0);
Copy link
Contributor

@s1monw s1monw Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am hesitating to set a 0 timeout for SO_LINGER The reason is that the abnormal termination of a socket that causes it to move into TIME_WAIT can be caused due to an exception ie. during request parsing on the server then the server closes the connection and in this case we should really stay in TIME_WAIT. The other case where the server closes the connection instead of the client is when we shutdown and then we can set our socket into SO_LINGER=0 since it's the right thing todo in this situation. I actually think we should do this in a broader scope. There is for instance a method TcpTransport#closeChannels(List<Channels> channels, boolean blocking) I wonder if we can piggyback on this boolean and set SO_LINGER to true,0 if we close our transport. It might even be a better idea to add another boolean boolean closingTransport in this case we can do the right thing also in other transport impls?

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s1monw

It might even be a better idea to add another boolean boolean closingTransport in this case we can do the right thing also in other transport impls?

Makes sense to me to do the additional boolean. My reasoning would be that the blocking parameter currently seems to be more about the way handle Netty's internal threading and not any low-level Socket behavior/setting.

Copy link
Contributor Author

@original-brownbear original-brownbear Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s1monw done, added another parameter and only made it RST on org.elasticsearch.transport.TcpTransport#doStop and the actual close call of TcpTransport leaving the failure handlers and such untouched.
This still (after removing the SO_LINGER setting I added) resolves the issue just fine, tests still don't leak any TIME_WAITs.
Hope this is what you had in mind :)

synchronized (this) {
if (isOpen.get()) {
incomingChannel = new MockChannel(incomingSocket,
Expand Down