BUG-9048 Fix actor crash when writing incorrect data

We need to catch all unechecked exceptions when writing data
in the clustered implementation since these will crash the actor
running and cause problems down the line.

Change-Id: Ib9d243d1db0f6e29e55c97f794a122f307420055
Signed-off-by: Tomas Cere <tcere@cisco.com>

diff --git a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteAdapter.java b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteAdapter.java
index 4cf51eead297a3fbaff603a23ab22df699227a68..ef5df778d630d4b26a844a86a7fac5f027498d0d 100644 (file)
--- a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteAdapter.java
+++ b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteAdapter.java
@@ -21,10 +21,16 @@ import org.opendaylight.netconf.topology.singleton.messages.transactions.CancelR
 import org.opendaylight.netconf.topology.singleton.messages.transactions.DeleteRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.MergeRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.PutRequest;
+import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitFailedReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 class WriteAdapter {
+
+    private static final Logger LOG = LoggerFactory.getLogger(WriteAdapter.class);
+
     private final DOMDataWriteTransaction tx;
 
     WriteAdapter(final DOMDataWriteTransaction tx) {
@@ -48,27 +54,35 @@ class WriteAdapter {
 
             @Override
             public void onFailure(@Nonnull final Throwable throwable) {
-                requester.tell(throwable, self);
+                requester.tell(new SubmitFailedReply(throwable), self);
             }
         });
     }
 
+    @SuppressWarnings("checkstyle:IllegalCatch")
     public void handle(final Object message, final ActorRef sender, final ActorContext context, final ActorRef self) {
-        if (message instanceof MergeRequest) {
-            final MergeRequest mergeRequest = (MergeRequest) message;
-            final NormalizedNodeMessage data = mergeRequest.getNormalizedNodeMessage();
-            tx.merge(mergeRequest.getStore(), data.getIdentifier(), data.getNode());
-        } else if (message instanceof PutRequest) {
-            final PutRequest putRequest = (PutRequest) message;
-            final NormalizedNodeMessage data = putRequest.getNormalizedNodeMessage();
-            tx.put(putRequest.getStore(), data.getIdentifier(), data.getNode());
-        } else if (message instanceof DeleteRequest) {
-            final DeleteRequest deleteRequest = (DeleteRequest) message;
-            tx.delete(deleteRequest.getStore(), deleteRequest.getPath());
-        } else if (message instanceof CancelRequest) {
-            cancel(context, sender, self);
-        } else if (message instanceof SubmitRequest) {
-            submit(sender, self, context);
+        // we need to catch everything, since an unchecked exception can be thrown from the underlying parse.
+        // TODO Maybe we should store it and fail the submit immediately?.
+        try {
+            if (message instanceof MergeRequest) {
+                final MergeRequest mergeRequest = (MergeRequest) message;
+                final NormalizedNodeMessage data = mergeRequest.getNormalizedNodeMessage();
+                tx.merge(mergeRequest.getStore(), data.getIdentifier(), data.getNode());
+            } else if (message instanceof PutRequest) {
+                final PutRequest putRequest = (PutRequest) message;
+                final NormalizedNodeMessage data = putRequest.getNormalizedNodeMessage();
+                tx.put(putRequest.getStore(), data.getIdentifier(), data.getNode());
+            } else if (message instanceof DeleteRequest) {
+                final DeleteRequest deleteRequest = (DeleteRequest) message;
+                tx.delete(deleteRequest.getStore(), deleteRequest.getPath());
+            } else if (message instanceof CancelRequest) {
+                cancel(context, sender, self);
+            } else if (message instanceof SubmitRequest) {
+                submit(sender, self, context);
+            }
+
+        } catch (final RuntimeException exception) {
+            LOG.error("Write command has failed.", exception);
         }
     }
 }

diff --git a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/tx/ProxyWriteAdapter.java b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/tx/ProxyWriteAdapter.java
index 223e443cd52161228be1a7755cc168bd6bcfd77a..a833e3bb422ef721030ea426beab4c45ee40c08f 100644 (file)
--- a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/tx/ProxyWriteAdapter.java
+++ b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/impl/tx/ProxyWriteAdapter.java
@@ -101,7 +101,10 @@ public class ProxyWriteAdapter {
                 } else {
                     if (success instanceof SubmitFailedReply) {
                         LOG.error("{}: Transaction was not submitted because already closed.", id);
+                        settableFuture.setException(((SubmitFailedReply) success).getThrowable());
+                        return;
                     }
+
                     settableFuture.set(null);
                 }
             }

diff --git a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/messages/transactions/SubmitFailedReply.java b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/messages/transactions/SubmitFailedReply.java
index 18e1f47393c01493398e9304114e6e4e08ac4d5c..7dfc19c0c6e79a000e9cb9db43c83e5e4d360b3b 100644 (file)
--- a/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/messages/transactions/SubmitFailedReply.java
+++ b/netconf/netconf-topology-singleton/src/main/java/org/opendaylight/netconf/topology/singleton/messages/transactions/SubmitFailedReply.java
@@ -11,8 +11,18 @@ package org.opendaylight.netconf.topology.singleton.messages.transactions;
 import java.io.Serializable;
 
 /**
- * Message sent from master back to the slave when submit is not performed, tx is closed.
+ * Message sent from master back to the slave when submit fails, with the offending exception attached.
  */
 public class SubmitFailedReply implements Serializable {
     private static final long serialVersionUID = 1L;
+
+    private final Throwable throwable;
+
+    public SubmitFailedReply(final Throwable throwable) {
+        this.throwable = throwable;
+    }
+
+    public Throwable getThrowable() {
+        return throwable;
+    }
 }

diff --git a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/ReadWriteTransactionActorTest.java b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/ReadWriteTransactionActorTest.java
index e02f1f8dc72a6bce4fbfe9230988ae01221ce52f..e9eee1851a2aaf77d286679927df98f303c05869 100644 (file)
--- a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/ReadWriteTransactionActorTest.java
+++ b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/ReadWriteTransactionActorTest.java
@@ -39,6 +39,7 @@ import org.opendaylight.netconf.topology.singleton.messages.transactions.ExistsR
 import org.opendaylight.netconf.topology.singleton.messages.transactions.MergeRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.PutRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.ReadRequest;
+import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitFailedReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitRequest;
 import org.opendaylight.yangtools.yang.common.QName;
@@ -174,7 +175,8 @@ public class ReadWriteTransactionActorTest {
         when(deviceReadWriteTx.submit()).thenReturn(Futures.immediateFailedCheckedFuture(cause));
         final Future<Object> submitFuture = Patterns.ask(actorRef, new SubmitRequest(), TIMEOUT);
         final Object result = Await.result(submitFuture, TIMEOUT.duration());
-        Assert.assertEquals(cause, result);
+        Assert.assertTrue(result instanceof SubmitFailedReply);
+        Assert.assertEquals(cause, ((SubmitFailedReply)result).getThrowable());
         verify(deviceReadWriteTx).submit();
     }
 

diff --git a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteTransactionActorTest.java b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteTransactionActorTest.java
index 4e7fc3537b5e006a837026aa11355106b17af587..cd2b9829142bc96d15a44bbb1066eb1a81d12262 100644 (file)
--- a/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteTransactionActorTest.java
+++ b/netconf/netconf-topology-singleton/src/test/java/org/opendaylight/netconf/topology/singleton/impl/actors/WriteTransactionActorTest.java
@@ -34,6 +34,7 @@ import org.opendaylight.netconf.topology.singleton.messages.transactions.CancelR
 import org.opendaylight.netconf.topology.singleton.messages.transactions.DeleteRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.MergeRequest;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.PutRequest;
+import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitFailedReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitReply;
 import org.opendaylight.netconf.topology.singleton.messages.transactions.SubmitRequest;
 import org.opendaylight.yangtools.yang.common.QName;
@@ -122,7 +123,8 @@ public class WriteTransactionActorTest {
         when(deviceWriteTx.submit()).thenReturn(Futures.immediateFailedCheckedFuture(cause));
         final Future<Object> submitFuture = Patterns.ask(actorRef, new SubmitRequest(), TIMEOUT);
         final Object result = Await.result(submitFuture, TIMEOUT.duration());
-        Assert.assertEquals(cause, result);
+        Assert.assertTrue(result instanceof SubmitFailedReply);
+        Assert.assertEquals(cause, ((SubmitFailedReply)result).getThrowable());
         verify(deviceWriteTx).submit();
     }