Properly clean up a partially established connection (#2428)

winder · Jan 16, 2024 · 90a735a · 90a735a
1 parent 15c7e72
commit 90a735a
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 61 deletions.
diff --git a/pom.xml b/pom.xml
@@ -55,7 +55,7 @@
     <miglayout.version>3.7.4</miglayout.version>
     <guava.version>28.1-jre</guava.version>
     <jssc.version>2.8.0</jssc.version>
-    <jserialcomm.version>2.9.2</jserialcomm.version>
+    <jserialcomm.version>2.10.4</jserialcomm.version>
     <commons-lang3.version>3.12.0</commons-lang3.version>
     <commons-io.version>2.14.0</commons-io.version>
     <commons-csv.version>1.9.0</commons-csv.version>

diff --git a/ugs-core/src/com/willwinder/universalgcodesender/communicator/AbstractCommunicator.java b/ugs-core/src/com/willwinder/universalgcodesender/communicator/AbstractCommunicator.java
@@ -1,5 +1,5 @@
 /*
-    Copyright 2013-2018 Will Winder
+    Copyright 2013-2024 Will Winder
 
     This file is part of Universal Gcode Sender (UGS).
 
@@ -63,7 +63,9 @@ public void resetBuffers() {
     @Override
     public void setConnection(Connection c) {
         connection = c;
-        c.addListener(this);
+        if (c != null) {
+            c.addListener(this);
+        }
     }
 
     //do common operations (related to the connection, that is shared by all communicators)
@@ -75,15 +77,12 @@ public void connect(ConnectionDriver connectionDriver, String name, int baud) th
             logger.info("Connecting to controller using class: " + connection.getClass().getSimpleName() + " with url " + url);
         }
 
-        if (connection != null) {
-            connection.addListener(this);
-        }
-
+        // Abort if we still have not got a connection
         if (connection == null) {
             throw new Exception(Localization.getString("communicator.exception.port") + ": " + name);
         }
 
-        //open it
+        connection.addListener(this);
         if (!connection.openPort()) {
             throw new Exception("Could not connect to controller on port " + url);
         }
@@ -99,7 +98,9 @@ public boolean isConnected() {
     @Override
     public void disconnect() throws Exception {
         eventDispatcher.reset();
-        connection.closePort();
+        if (connection != null) {
+            connection.closePort();
+        }
     }
 
     /* ****************** */

diff --git a/ugs-core/src/com/willwinder/universalgcodesender/model/GUIBackend.java b/ugs-core/src/com/willwinder/universalgcodesender/model/GUIBackend.java
@@ -1,5 +1,5 @@
 /*
-    Copyright 2015-2023 Will Winder
+    Copyright 2015-2024 Will Winder
 
     This file is part of Universal Gcode Sender (UGS).
 
@@ -755,6 +755,7 @@ private boolean openCommConnection(String port, int baudRate) throws Exception {
 
             this.initializeProcessedLines(false, this.gcodeFile, this.gcp);
         } catch (Exception e) {
+            disconnect();
             logger.log(Level.INFO, "Exception in openCommConnection.", e);
             throw new Exception(Localization.getString("mainWindow.error.connection")
                     + ": " + e.getMessage());

diff --git a/ugs-core/test/com/willwinder/universalgcodesender/communicator/BufferedCommunicatorTest.java b/ugs-core/test/com/willwinder/universalgcodesender/communicator/BufferedCommunicatorTest.java
@@ -1,5 +1,5 @@
 /*
-    Copyright 2015-2018 Will Winder
+    Copyright 2015-2024 Will Winder
 
     This file is part of Universal Gcode Sender (UGS).
 
@@ -31,24 +31,23 @@ This file is part of Universal Gcode Sender (UGS).
 import org.apache.commons.io.FileUtils;
 import org.easymock.EasyMock;
 import org.junit.AfterClass;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 import java.io.File;
 import java.io.IOException;
 import java.util.concurrent.LinkedBlockingDeque;
 import java.util.concurrent.atomic.AtomicBoolean;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.doNothing;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-
 /**
  * @author wwinder
  */
@@ -91,7 +90,6 @@ public void setUp() throws IllegalArgumentException, IllegalAccessException, NoS
      */
     @Test
     public void testGetBufferSize() {
-        System.out.println("getBufferSize");
         assertEquals(101, instance.getBufferSize());
     }
 
@@ -100,7 +98,6 @@ public void testGetBufferSize() {
      */
     @Test
     public void testQueueStringForComm() {
-        System.out.println("queueStringForComm");
         String input = "input";
         instance.queueCommand(new GcodeCommand(input));
         assertEquals(input, cb.getFirst().getCommandString());
@@ -111,8 +108,6 @@ public void testQueueStringForComm() {
      */
     @Test
     public void testSimpleQueueStringsStream() throws Exception {
-        System.out.println("streamCommands");
-
         String input = "input";
 
         // Check events and connection:
@@ -333,13 +328,8 @@ public void testOpenCommPort() throws Exception {
         EasyMock.verify(mockConnection);
     }
 
-    /**
-     * Test of closeCommPort method, of class BufferedCommunicator.
-     */
     @Test
-    public void testCloseCommPort() throws Exception {
-        System.out.println("closeCommPort");
-
+    public void disconnectShouldBeOk() throws Exception {
         mockConnection.closePort();
         EasyMock.expect(EasyMock.expectLastCall()).once();
         EasyMock.replay(mockConnection);
@@ -349,12 +339,35 @@ public void testCloseCommPort() throws Exception {
         EasyMock.verify(mockConnection);
     }
 
+    @Test
+    public void disconnectShouldClearCommandBuffers() throws Exception {
+        cb.add(new GcodeCommand("test1"));
+        asl.add(new GcodeCommand("test2"));
+
+        instance.disconnect();
+
+        assertTrue("Expected the command buffer to have been cleared", cb.isEmpty());
+        assertTrue("Expected the active command list to have been cleared", asl.isEmpty());
+    }
+
+    @Test
+    public void disconnectWithoutConnectionShouldBeOk() throws Exception {
+        cb.add(new GcodeCommand("test1"));
+        asl.add(new GcodeCommand("test2"));
+        instance.setConnection(null);
+
+        instance.disconnect();
+
+        assertFalse(instance.isPaused());
+        assertTrue("Expected the command buffer to have been cleared", cb.isEmpty());
+        assertTrue("Expected the active command list to have been cleared", asl.isEmpty());
+    }
+
     /**
      * Test of sendByteImmediately method, of class BufferedCommunicator.
      */
     @Test
     public void testSendByteImmediately() throws Exception {
-        System.out.println("sendByteImmediately");
         byte b = 10;
 
         String tenChar = "123456789";
@@ -377,24 +390,6 @@ public void testSendByteImmediately() throws Exception {
         EasyMock.verify(mockConnection);
     }
 
-    /**
-     * Test of sendingCommand method, of class BufferedCommunicatorImpl.
-     */
-    @Test
-    public void testSendingCommand() {
-        System.out.println("sendingCommand");
-        System.out.println("-N/A for abstract class-");
-    }
-
-    /**
-     * Test of processedCommand method, of class BufferedCommunicatorImpl.
-     */
-    @Test
-    public void testProcessedCommand() {
-        System.out.println("processedCommand");
-        System.out.println("-N/A for abstract class-");
-    }
-
     @Test
     public void testStreamCommandsOrderStringCommandsFirst() throws Exception {
         // Given

diff --git a/ugs-core/test/com/willwinder/universalgcodesender/model/GUIBackendTest.java b/ugs-core/test/com/willwinder/universalgcodesender/model/GUIBackendTest.java
@@ -1,5 +1,5 @@
 /*
-    Copyright 2016-2018 Will Winder
+    Copyright 2016-2024 Will Winder
 
     This file is part of Universal Gcode Sender (UGS).
 
@@ -31,21 +31,16 @@ This file is part of Universal Gcode Sender (UGS).
 import com.willwinder.universalgcodesender.types.GcodeCommand;
 import com.willwinder.universalgcodesender.utils.Settings;
 import org.apache.commons.io.FileUtils;
-import org.junit.Before;
-import org.junit.Test;
-import org.mockito.ArgumentCaptor;
-
-import java.io.File;
-import java.io.IOException;
-import java.nio.charset.StandardCharsets;
-import java.util.List;
-
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyDouble;
 import static org.mockito.Mockito.anyString;
@@ -57,6 +52,11 @@ This file is part of Universal Gcode Sender (UGS).
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+
 /**
  * Unit test for GUIBackend
  *
@@ -264,10 +264,11 @@ public void connectWithUnknownFirmwareShouldNotBeOk() throws Exception {
         instance.connect("unknown", PORT, BAUD_RATE);
     }
 
-    @Test(expected = Exception.class)
-    public void connectWhenFailingToOpenControllerConnectionShouldNotBeOk() throws Exception {
+    @Test
+    public void connectWhenFailingToOpenControllerConnectionShouldNotBeOkAndDisconnect() throws Exception {
         when(controller.openCommPort(settings.getConnectionDriver(), PORT, BAUD_RATE)).thenThrow(new Exception());
-        instance.connect(FIRMWARE, PORT, BAUD_RATE);
+        assertThrows(Exception.class, () -> instance.connect(FIRMWARE, PORT, BAUD_RATE));
+        verify(instance, times(1)).disconnect();
     }
 
     @Test