From 90a735a2bdff5a1d254a2bbd56838ee7e9a6ddcb Mon Sep 17 00:00:00 2001 From: Joacim Breiler Date: Tue, 16 Jan 2024 12:56:24 +0100 Subject: [PATCH] Properly clean up a partially established connection (#2428) --- pom.xml | 2 +- .../communicator/AbstractCommunicator.java | 17 ++--- .../model/GUIBackend.java | 3 +- .../BufferedCommunicatorTest.java | 71 +++++++++---------- .../model/GUIBackendTest.java | 27 +++---- 5 files changed, 59 insertions(+), 61 deletions(-) diff --git a/pom.xml b/pom.xml index 0acabcab61..99051e151a 100644 --- a/pom.xml +++ b/pom.xml @@ -55,7 +55,7 @@ 3.7.4 28.1-jre 2.8.0 - 2.9.2 + 2.10.4 3.12.0 2.14.0 1.9.0 diff --git a/ugs-core/src/com/willwinder/universalgcodesender/communicator/AbstractCommunicator.java b/ugs-core/src/com/willwinder/universalgcodesender/communicator/AbstractCommunicator.java index a9819ae68a..d9be80f242 100644 --- 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 index 1a00999d90..5c597327d4 100644 --- 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 index dcec82cda5..2fad450875 100644 --- 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 index 1cb1530018..705395dcd6 100644 --- 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