Skip to content
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.

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

Ensure new connections won't be opened if transport is closed or closing #22589

Merged
merged 3 commits into from Jan 12, 2017

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Jan 12, 2017

Today there are several races / holes in TcpTransport and MockTcpTransport
that can allow connections to be opened and remain unclosed while the actual
transport implementation is closed. A recently added assertions in #22554 exposes
these problems. This commit fixes several issues related to missed locks or channel
creations outside of a lock not checking if the resource is still open.

Ensure new connections won't be opened if transport is closed or closing
Today there are several races / holes in TcpTransport and MockTcpTransport
that can allow connections to be opened and remain unclosed while the actual
transport implementation is closed. A recently added assertions in #22554 exposes
these problems. This commit fixes several issues related to missed locks or channel
creations outside of a lock not checking if the resource is still open.
@jasontedor
Copy link
Member

left a comment

@s1monw It looks good, I left two suggestions.

* Ensures this transport is still started / open
* @throws IllegalStateException if the transport is not started / open
*/
protected final void ensureOpen() {

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 12, 2017

Member

Assert that the lock is held?

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 12, 2017

Author Contributor

I don't think it needs to hold a lock here. it's really best effort and can be used anywhere. Under lock it has different semantics but they are related to the context of the lock so I think we are good

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 12, 2017

Member

I guess I'm missing something, but you're checking that we are indeed open; if we don't hold the lock here aren't we subject to time-of-check-time-of-use problems?

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 12, 2017

Author Contributor

again, the ensureOpen is only a check to fail if we are closed. Yet, if you run it under the lock you make sure that nobody else is modifying a structure protected by this lock. The state of being open / closed is not protected by this lock. It's a volatile read that's it. I don't understand what your issue is here to be honest. The lock has nothing todo with the ensureOpen call. We do this in many other places and almost never protect that once the check has passed a close can't be concurrently happen. For the check itself it doesn't matter, it might matter for the caller but the callers context can have this extra protection. In this case here all places where it's used are accidentally protected by a lock but that is not required for this check.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 12, 2017

Member

I'm sorry that I misread it.

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 12, 2017

Author Contributor

I guess this can happen to others... I am going to add a comment

@@ -370,9 +373,20 @@ public void close() throws IOException {
IOUtils.close(serverSocket, activeChannel, () -> IOUtils.close(workerChannels.keySet()),
() -> cancellableThreads.cancel("channel closed"), onClose);
}
assert removedChannel : "Channel was not removed or removed twice?";
// assert remoteChannel; is not enough it will throw NPE if removeChannel is null
assert removedChannel == Boolean.TRUE: "Channel was not removed or removed twice?";

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 12, 2017

Member

I think that removedChannel != null && removalChannel is more defensive (here we are relying on either always putting Boolean.TRUE in the map (which we do), or the fact that a boxed boolean always boxes to Boolean.TRUE (the JVM guarantees this) but I prefer to not rely on these fragile/subtle things).

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 12, 2017

Author Contributor

I just moved to use a set instead.

@jasontedor
Copy link
Member

left a comment

LGTM.

@@ -76,7 +78,7 @@
*/
public static final ConnectionProfile LIGHT_PROFILE;

private final Map<MockChannel, Boolean> openChannels = new IdentityHashMap<>();
private final Set<MockChannel> openChannels = new HashSet<>();

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 12, 2017

Member

That's even better.

@s1monw s1monw merged commit acf2d2f into elastic:master Jan 12, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:enhance_closing_in_tcp_transport branch Jan 12, 2017

s1monw added a commit that referenced this pull request Jan 12, 2017

Ensure new connections won't be opened if transport is closed or clos…
…ing (#22589)

Today there are several races / holes in TcpTransport and MockTcpTransport
that can allow connections to be opened and remain unclosed while the actual
transport implementation is closed. A recently added assertions in #22554 exposes
these problems. This commit fixes several issues related to missed locks or channel
creations outside of a lock not checking if the resource is still open.

s1monw added a commit that referenced this pull request Jan 13, 2017

Ensure new connections won't be opened if transport is closed or clos…
…ing (#22589)

Today there are several races / holes in TcpTransport and MockTcpTransport
that can allow connections to be opened and remain unclosed while the actual
transport implementation is closed. A recently added assertions in #22554 exposes
these problems. This commit fixes several issues related to missed locks or channel
creations outside of a lock not checking if the resource is still open.

@clintongormley clintongormley removed the v5.3.0 label Jan 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.