qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rob...@apache.org
Subject qpid-proton-j git commit: PROTON-1938: align TransportImpl set/getCondition behaviour, plus stop the closed method replacing an existing ErrorCondition
Date Tue, 30 Oct 2018 14:57:32 GMT
Repository: qpid-proton-j
Updated Branches:
  refs/heads/master 1a5e19369 -> 19bf22f7e


PROTON-1938: align TransportImpl set/getCondition behaviour, plus stop the closed method replacing
an existing ErrorCondition


Project: http://git-wip-us.apache.org/repos/asf/qpid-proton-j/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton-j/commit/19bf22f7
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton-j/tree/19bf22f7
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton-j/diff/19bf22f7

Branch: refs/heads/master
Commit: 19bf22f7e9b88e73db4b195f674d1527dbd713f3
Parents: 1a5e193
Author: Robbie Gemmell <robbie@apache.org>
Authored: Tue Oct 30 14:47:44 2018 +0000
Committer: Robbie Gemmell <robbie@apache.org>
Committed: Tue Oct 30 14:47:44 2018 +0000

----------------------------------------------------------------------
 .../qpid/proton/engine/impl/TransportImpl.java  |  39 ++++---
 .../proton/engine/impl/TransportImplTest.java   | 107 +++++++++++++++++++
 2 files changed, 133 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-proton-j/blob/19bf22f7/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/TransportImpl.java
----------------------------------------------------------------------
diff --git a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/TransportImpl.java
b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/TransportImpl.java
index 8947fa3..e07edbb 100644
--- a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/TransportImpl.java
+++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/TransportImpl.java
@@ -125,7 +125,7 @@ public class TransportImpl extends EndpointImpl
 
     private FrameHandler _frameHandler = this;
     private boolean _head_closed = false;
-    private ErrorCondition _condition = null;
+    private boolean _conditionSet;
 
     private boolean postedHeadClosed = false;
     private boolean postedTailClosed = false;
@@ -250,7 +250,19 @@ public class TransportImpl extends EndpointImpl
     @Override
     public ErrorCondition getCondition()
     {
-        return _condition;
+        // Get the ErrorCondition, but only return it if its condition field is populated.
+        // This somewhat retains prior TransportImpl behaviour of returning null when no
+        // condition had been set (by TransportImpl itself) rather than the 'empty' ErrorCondition
+        // object historically used in the other areas.
+        ErrorCondition errorCondition = super.getCondition();
+        return errorCondition.getCondition() != null ? errorCondition : null;
+    }
+
+    @Override
+    public void setCondition(ErrorCondition error)
+    {
+        super.setCondition(error);
+        _conditionSet = error != null && error.getCondition() != null;
     }
 
     @Override
@@ -844,7 +856,7 @@ public class TransportImpl extends EndpointImpl
 
     private void processOpen()
     {
-        if (!_isOpenSent && (_condition != null ||
+        if (!_isOpenSent && (_conditionSet ||
              (_connectionEndpoint != null &&
               _connectionEndpoint.getLocalState() != EndpointState.UNINITIALIZED)))
         {
@@ -1042,7 +1054,7 @@ public class TransportImpl extends EndpointImpl
 
     private void processClose()
     {
-        if ((_condition != null ||
+        if ((_conditionSet ||
              (_connectionEndpoint != null &&
               _connectionEndpoint.getLocalState() == EndpointState.CLOSED)) &&
             !_isCloseSent) {
@@ -1053,12 +1065,12 @@ public class TransportImpl extends EndpointImpl
                 ErrorCondition localError;
 
                 if (_connectionEndpoint == null) {
-                    localError = _condition;
+                    localError = getCondition();
                 } else {
                     localError =  _connectionEndpoint.getCondition();
                 }
 
-                if(localError.getCondition() != null)
+                if(localError != null && localError.getCondition() != null)
                 {
                     close.setError(localError);
                 }
@@ -1419,19 +1431,20 @@ public class TransportImpl extends EndpointImpl
     public void closed(TransportException error)
     {
         if (!_closeReceived || error != null) {
-            if (error == null) {
-                _condition = new ErrorCondition(ConnectionError.FRAMING_ERROR,
-                                               "connection aborted");
-            } else {
-                _condition = new ErrorCondition(ConnectionError.FRAMING_ERROR,
-                                                error.toString());
+            // Set an error condition, but only if one was not already set
+            if(!_conditionSet) {
+                String description =  error == null ? "connection aborted" : error.toString();
+                setCondition(new ErrorCondition(ConnectionError.FRAMING_ERROR, description));
             }
+
             _head_closed = true;
         }
-        if (_condition != null && !postedTransportError) {
+
+        if (_conditionSet && !postedTransportError) {
             put(Event.Type.TRANSPORT_ERROR, this);
             postedTransportError = true;
         }
+
         if (!postedTailClosed) {
             put(Event.Type.TRANSPORT_TAIL_CLOSED, this);
             postedTailClosed = true;

http://git-wip-us.apache.org/repos/asf/qpid-proton-j/blob/19bf22f7/proton-j/src/test/java/org/apache/qpid/proton/engine/impl/TransportImplTest.java
----------------------------------------------------------------------
diff --git a/proton-j/src/test/java/org/apache/qpid/proton/engine/impl/TransportImplTest.java
b/proton-j/src/test/java/org/apache/qpid/proton/engine/impl/TransportImplTest.java
index fbc9b8f..203274f 100644
--- a/proton-j/src/test/java/org/apache/qpid/proton/engine/impl/TransportImplTest.java
+++ b/proton-j/src/test/java/org/apache/qpid/proton/engine/impl/TransportImplTest.java
@@ -24,6 +24,7 @@ import static org.apache.qpid.proton.engine.impl.TransportTestHelper.stringOfLen
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -38,6 +39,7 @@ import java.util.Random;
 
 import org.apache.qpid.proton.Proton;
 import org.apache.qpid.proton.amqp.Binary;
+import org.apache.qpid.proton.amqp.Symbol;
 import org.apache.qpid.proton.amqp.UnsignedInteger;
 import org.apache.qpid.proton.amqp.UnsignedShort;
 import org.apache.qpid.proton.amqp.messaging.Accepted;
@@ -46,9 +48,11 @@ import org.apache.qpid.proton.amqp.messaging.Released;
 import org.apache.qpid.proton.amqp.transport.Attach;
 import org.apache.qpid.proton.amqp.transport.Begin;
 import org.apache.qpid.proton.amqp.transport.Close;
+import org.apache.qpid.proton.amqp.transport.ConnectionError;
 import org.apache.qpid.proton.amqp.transport.Detach;
 import org.apache.qpid.proton.amqp.transport.Disposition;
 import org.apache.qpid.proton.amqp.transport.End;
+import org.apache.qpid.proton.amqp.transport.ErrorCondition;
 import org.apache.qpid.proton.amqp.transport.Flow;
 import org.apache.qpid.proton.amqp.transport.FrameBody;
 import org.apache.qpid.proton.amqp.transport.Open;
@@ -3530,4 +3534,107 @@ public class TransportImplTest
         assertEquals("Unexpected delivery count", UnsignedInteger.valueOf(3), sentFlow.getDeliveryCount());
         assertEquals("Unexpected credit", UnsignedInteger.valueOf(123), sentFlow.getLinkCredit());
     }
+
+    @Test
+    public void testErrorConditionDefault() {
+        TransportImpl transport = new TransportImpl();
+        assertNull("Expected null ErrorCondition given historic behaviour", transport.getCondition());
+    }
+
+    @Test
+    public void testErrorConditionSetGet() {
+        // Try setting with an empty condition object, expect to get a null back per historic
behaviour.
+        TransportImpl transport = new TransportImpl();
+
+        ErrorCondition emptyErrorCondition = new ErrorCondition();
+        assertNull("Expected empty Condition given historic behaviour", emptyErrorCondition.getCondition());
+        transport.setCondition(emptyErrorCondition);
+        assertNull("Expected null ErrorCondition given historic behaviour", transport.getCondition());
+
+        // Try setting with a populated condition object.
+        transport = new TransportImpl();
+
+        Symbol condition = Symbol.getSymbol("some-error");
+        String description = "some-error-description";
+        ErrorCondition populatedErrorCondition = new ErrorCondition();
+        populatedErrorCondition.setCondition(condition);
+        populatedErrorCondition.setDescription(description);
+        assertNotNull("Expected a Condition", populatedErrorCondition.getCondition());
+
+        transport.setCondition(populatedErrorCondition);
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected ErrorCondition returned", populatedErrorCondition, transport.getCondition());
+
+        // Try setting again with another populated condition object.
+        Symbol otherCondition = Symbol.getSymbol("some-other-error");
+        String otherDescription = "some-other-error-description";
+        ErrorCondition otherErrorCondition = new ErrorCondition();
+        otherErrorCondition.setCondition(otherCondition);
+        otherErrorCondition.setDescription(otherDescription);
+        assertNotNull("Expected a Condition", otherErrorCondition.getCondition());
+
+        assertNotEquals(condition, otherCondition);
+        assertNotEquals(populatedErrorCondition.getCondition(), otherErrorCondition.getCondition());
+        assertNotEquals(description, otherDescription);
+        assertNotEquals(populatedErrorCondition.getDescription(), otherErrorCondition.getDescription());
+        assertNotEquals(populatedErrorCondition, otherErrorCondition);
+
+        transport.setCondition(otherErrorCondition);
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected ErrorCondition returned", otherErrorCondition, transport.getCondition());
+
+        // Try setting again with an empty condition object, expect to get a null back per
historic behaviour.
+        transport.setCondition(emptyErrorCondition);
+        assertNull("Expected null ErrorCondition given historic behaviour", transport.getCondition());
+    }
+
+    @Test
+    public void testErrorConditionAfterTransportClosed() {
+        Symbol condition = Symbol.getSymbol("some-error");
+        String description = "some-error-description";
+        ErrorCondition origErrorCondition = new ErrorCondition();
+        origErrorCondition.setCondition(condition);
+        origErrorCondition.setDescription(description);
+        assertNotNull("Expected a Condition", origErrorCondition.getCondition());
+
+        // Set an error condition, then call 'closed' specifying an error.
+        // Expect the original condition which was set to remain.
+        TransportImpl transport = new TransportImpl();
+
+        transport.setCondition(origErrorCondition);
+        transport.closed(new TransportException("my-ignored-exception"));
+
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected ErrorCondition returned", origErrorCondition, transport.getCondition());
+
+        // Set an error condition, then call 'closed' without an error.
+        // Expect the original condition which was set to remain.
+        transport = new TransportImpl();
+
+        transport.setCondition(origErrorCondition);
+        transport.closed(null);
+
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected ErrorCondition returned", origErrorCondition, transport.getCondition());
+
+        // Without having set an error condition, call 'closed' specifying an error.
+        // Expect a condition to be set.
+        String errorDescription = "some-error-description";
+        transport = new TransportImpl();
+        transport.closed(new TransportException(errorDescription));
+
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected condition returned", ConnectionError.FRAMING_ERROR, transport.getCondition().getCondition());
+        assertEquals("Unexpected description returned", "org.apache.qpid.proton.engine.TransportException:
" + errorDescription, transport.getCondition().getDescription());
+
+        // Without having set an error condition, call 'closed' without an error.
+        // Expect a condition to be set.
+        transport = new TransportImpl();
+
+        transport.closed(null);
+
+        assertNotNull("Expected an ErrorCondition to be returned", transport.getCondition());
+        assertEquals("Unexpected ErrorCondition returned", ConnectionError.FRAMING_ERROR,
transport.getCondition().getCondition());
+        assertEquals("Unexpected description returned", "connection aborted", transport.getCondition().getDescription());
+    }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message