You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2017/09/07 02:15:53 UTC
[1/2] kudu git commit: KUDU-2130: java client: handle termination
during negotiation edge case
Repository: kudu
Updated Branches:
refs/heads/branch-1.5.x 9101f85fa -> b31f3e5cc
KUDU-2130: java client: handle termination during negotiation edge case
There was an edge case where a Connection can be terminated while negotiation
is completing. This would result in a scary looking log message:
18:24:07.776 [DEBUG - New I/O worker #8112] (Connection.java:649) [peer master-127.32.133.1:64032] cleaning up while in state NEGOTIATING due to: connection disconnected
18:24:07.781 [ERROR - New I/O worker #8112] (Connection.java:418) [peer master-127.32.133.1:64032] unexpected exception from downstream on [id: 0xdd52bacc, /127.0.0.1:55318 :> /127.32.133.1:64032]
java.lang.NullPointerException
at org.apache.kudu.client.Connection.messageReceived(Connection.java:271)
at org.jboss.netty.channel.SimpleChannelUpstreamHandler.handleUpstream(SimpleChannelUpstreamHandler.java:70)
at org.apache.kudu.client.Connection.handleUpstream(Connection.java:236)
at org.jboss.netty.channel.DefaultChannelPipeline.sendUpstream(DefaultChannelPipeline.java:564)
at org.jboss.netty.channel.DefaultChannelPipeline$DefaultChannelHandlerContext.sendUpstream(DefaultChannelPipeline.java:791)
but in reality the error message is harmless; it just indicates that the
connection has been terminated while the connection's messageReceived handler
is clearing the message queue. This interruption is possible because of
82a8e9f99, which fixed a deadlock in Connection. This commit recognizes when
this race has occured, and early exits from messageReceived.
Change-Id: I3e9d4a6535ae82973743e4ac1071de0aac92b08b
Reviewed-on: http://gerrit.cloudera.org:8080/7960
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
(cherry picked from commit f0aa3b3f194146760597e6ab88c304c6f408073c)
Reviewed-on: http://gerrit.cloudera.org:8080/7978
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/89677d83
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/89677d83
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/89677d83
Branch: refs/heads/branch-1.5.x
Commit: 89677d83c8bb3a1697fe95560b3b71634ee94dd4
Parents: 9101f85
Author: Dan Burkert <da...@apache.org>
Authored: Tue Sep 5 13:39:06 2017 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Thu Sep 7 02:10:18 2017 +0000
----------------------------------------------------------------------
.../main/java/org/apache/kudu/client/Connection.java | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/89677d83/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java b/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
index f21d2bb..8b1febb 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
@@ -261,14 +261,14 @@ class Connection extends SimpleChannelUpstreamHandler {
if (m instanceof Negotiator.Success) {
lock.lock();
try {
- Preconditions.checkState(state == State.NEGOTIATING);
- Preconditions.checkState(inflightMessages.isEmpty());
negotiationResult = (Negotiator.Success) m;
+ Preconditions.checkState(state == State.TERMINATED || inflightMessages.isEmpty());
// Before switching to the READY state, it's necessary to empty the queuedMessages. There
// might be concurrent activity on adding new messages into the queue if enqueueMessage()
// is called in the middle.
- while (!queuedMessages.isEmpty()) {
+ while (state != State.TERMINATED && !queuedMessages.isEmpty()) {
+
// Register the messages into the inflightMessages before sending them to the wire. This
// is to be able to invoke appropriate callback when the response received. This should
// be done under the lock since the inflightMessages itself does not provide any
@@ -292,8 +292,13 @@ class Connection extends SimpleChannelUpstreamHandler {
lock.lock();
}
}
+ // The connection may have been terminated while the lock was dropped.
+ if (state == State.TERMINATED) {
+ return;
+ }
+
+ Preconditions.checkState(state == State.NEGOTIATING);
- assert queuedMessages.isEmpty();
queuedMessages = null;
// Set the state to READY -- that means the incoming messages should be no longer put into
// the queuedMessages, but sent to wire right away (see the enqueueMessage() for details).
[2/2] kudu git commit: KUDU-2130 (part 2): more fixes for
ITClientStress
Posted by ad...@apache.org.
KUDU-2130 (part 2): more fixes for ITClientStress
This fixes some more race conditions in connection termination in the
same vein as part 1. It also filters benign SSLException from being
returned back to callers.
Change-Id: Ic3f518513931c660fd93b4272d1b1fceb268f191
Reviewed-on: http://gerrit.cloudera.org:8080/7964
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
(cherry picked from commit f41a5c2beeee3afc8b4703c8047b347490b24c19)
Reviewed-on: http://gerrit.cloudera.org:8080/7979
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b31f3e5c
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b31f3e5c
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b31f3e5c
Branch: refs/heads/branch-1.5.x
Commit: b31f3e5cc837dcd42f43ec90f0201480e15486a7
Parents: 89677d8
Author: Dan Burkert <da...@apache.org>
Authored: Tue Sep 5 15:39:36 2017 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Thu Sep 7 02:10:23 2017 +0000
----------------------------------------------------------------------
.../java/org/apache/kudu/client/Connection.java | 28 ++++++++++++++------
.../org/apache/kudu/client/ITClientStress.java | 2 ++
2 files changed, 22 insertions(+), 8 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/b31f3e5c/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java b/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
index 8b1febb..96395e4 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
@@ -214,6 +214,9 @@ class Connection extends SimpleChannelUpstreamHandler {
final ChannelStateEvent e) {
lock.lock();
try {
+ if (state == State.TERMINATED) {
+ return;
+ }
Preconditions.checkState(state == State.CONNECTING);
state = State.NEGOTIATING;
} finally {
@@ -313,6 +316,9 @@ class Connection extends SimpleChannelUpstreamHandler {
if (m instanceof Negotiator.Failure) {
lock.lock();
try {
+ if (state == State.TERMINATED) {
+ return;
+ }
Preconditions.checkState(state == State.NEGOTIATING);
Preconditions.checkState(inflightMessages.isEmpty());
@@ -347,6 +353,9 @@ class Connection extends SimpleChannelUpstreamHandler {
Callback<Void, CallResponseInfo> responseCbk;
lock.lock();
try {
+ if (state == State.TERMINATED) {
+ return;
+ }
Preconditions.checkState(state == State.READY);
responseCbk = inflightMessages.remove(callId);
} finally {
@@ -408,20 +417,23 @@ class Connection extends SimpleChannelUpstreamHandler {
getLogPrefix());
error = new RecoverableException(Status.NetworkError(message), e);
LOG.info(message, e);
- } else {
- String message = String.format("%s unexpected exception from downstream on %s",
- getLogPrefix(), c);
- error = new RecoverableException(Status.NetworkError(message), e);
-
+ } else if (e instanceof SSLException && explicitlyDisconnected) {
// There's a race in Netty where, when we call Channel.close(), it tries
// to send a TLS 'shutdown' message and enters a shutdown state. If another
// thread races to send actual data on the channel, then Netty will get a
// bit confused that we are trying to send data and misinterpret it as a
// renegotiation attempt, and throw an SSLException. So, we just ignore any
// SSLException if we've already attempted to close, otherwise log the error.
- if (!(e instanceof SSLException) || !explicitlyDisconnected) {
- LOG.error(message, e);
- }
+ error = new RecoverableException(Status.NetworkError(
+ String.format("%s disconnected from peer", getLogPrefix())));
+ } else {
+ // If the connection was explicitly disconnected via a call to disconnect(), we should
+ // have either gotten a ClosedChannelException or an SSLException.
+ assert !explicitlyDisconnected;
+ String message = String.format("%s unexpected exception from downstream on %s",
+ getLogPrefix(), c);
+ error = new RecoverableException(Status.NetworkError(message), e);
+ LOG.error(message, e);
}
cleanup(error);
http://git-wip-us.apache.org/repos/asf/kudu/blob/b31f3e5c/java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java b/java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
index a3632c9..71066b7 100644
--- a/java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
+++ b/java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java
@@ -80,6 +80,8 @@ public class ITClientStress extends BaseKuduTest {
cla.getAppendedText().contains("NullPointerException"));
assertFalse("log contained SSLException",
cla.getAppendedText().contains("SSLException"));
+ assertFalse("log contained IllegalStateException",
+ cla.getAppendedText().contains("IllegalStateException"));
}
/**