From c489bab9a13c2e87205515e639e769398833f6dd Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Mon, 3 Mar 2025 11:47:18 +0200 Subject: [PATCH 1/6] fix: clean up code and use try-with-resources for better resource management --- .../serial/internal/SerialFlashLaunch.java | 22 +--- .../serial/connector/SerialConnector.java | 42 ++++--- .../serial/connector/SerialPortHandler.java | 119 ++++++------------ .../launcher/SerialLauncherDelegate.java | 28 +++-- 4 files changed, 80 insertions(+), 131 deletions(-) diff --git a/bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java b/bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java index 7dd14ca3d..88d0acd7d 100644 --- a/bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java +++ b/bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunch.java @@ -15,10 +15,6 @@ *******************************************************************************/ package com.espressif.idf.launch.serial.internal; -import java.io.IOException; - -import org.eclipse.core.runtime.IStatus; -import org.eclipse.core.runtime.Status; import org.eclipse.debug.core.DebugEvent; import org.eclipse.debug.core.DebugPlugin; import org.eclipse.debug.core.ILaunchConfiguration; @@ -55,14 +51,7 @@ public void start() wasOpen = serialPort.isOpen(); if (wasOpen) { - try - { - serialPort.pause(); - } - catch (IOException e) - { - Activator.log(new Status(IStatus.ERROR, Activator.PLUGIN_ID, Messages.SerialFlashLaunch_Pause, e)); - } + serialPort.pause(); } } else @@ -77,14 +66,7 @@ public void handleDebugEvents(DebugEvent[] events) super.handleDebugEvents(events); if (isTerminated() && wasOpen) { - try - { - serialPort.resume(); - } - catch (IOException e) - { - Activator.log(new Status(IStatus.ERROR, Activator.PLUGIN_ID, Messages.SerialFlashLaunch_Resume, e)); - } + serialPort.resume(); wasOpen = false; } } diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java index 07691b62f..f65f84aa7 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialConnector.java @@ -13,7 +13,6 @@ *******************************************************************************/ package com.espressif.idf.terminal.connector.serial.connector; -import java.io.IOException; import java.io.OutputStream; import java.util.HashSet; import java.util.Set; @@ -28,7 +27,8 @@ import com.espressif.idf.core.util.StringUtil; import com.espressif.idf.terminal.connector.serial.activator.Activator; -public class SerialConnector extends TerminalConnectorImpl { +public class SerialConnector extends TerminalConnectorImpl +{ private SerialSettings settings = new SerialSettings(); protected Process process; @@ -40,43 +40,51 @@ public class SerialConnector extends TerminalConnectorImpl { private static Set openPorts = new HashSet<>(); - public static boolean isOpen(String portName) { + public static boolean isOpen(String portName) + { return openPorts.contains(portName); } @Override - public OutputStream getTerminalToRemoteStream() { + public OutputStream getTerminalToRemoteStream() + { return process.getOutputStream(); } - public SerialSettings getSettings() { + public SerialSettings getSettings() + { return settings; } @Override - public String getSettingsSummary() { + public String getSettingsSummary() + { return settings.getSummary(); } @Override - public void load(ISettingsStore store) { + public void load(ISettingsStore store) + { settings.load(store); } @Override - public void save(ISettingsStore store) { + public void save(ISettingsStore store) + { settings.save(store); } @Override - public void connect(ITerminalControl control) { + public void connect(ITerminalControl control) + { super.connect(control); this.control = control; - //Get selected project - which is required for IDF Monitor + // Get selected project - which is required for IDF Monitor project = settings.getProject(); - if (project == null) { + if (project == null) + { String message = "project can't be null. Make sure you select a project before launch a serial monitor"; //$NON-NLS-1$ Activator.log(new Status(IStatus.ERROR, Activator.getUniqueIdentifier(), message, null)); return; @@ -94,15 +102,13 @@ public void connect(ITerminalControl control) { } @Override - protected void doDisconnect() { + protected void doDisconnect() + { - if (serialPort != null && serialPort.isOpen()) { + if (serialPort != null && serialPort.isOpen()) + { openPorts.remove(serialPort.getPortName()); - try { - serialPort.close(); - } catch (IOException e) { - Activator.log(e); - } + serialPort.close(); } } diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index 10846d849..4efeb1de5 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -1,12 +1,10 @@ package com.espressif.idf.terminal.connector.serial.connector; -import java.io.IOException; import java.io.InputStream; -import java.lang.ref.WeakReference; -import java.util.HashMap; import java.util.Iterator; import java.util.LinkedList; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.eclipse.core.resources.IProject; import org.eclipse.tm.internal.terminal.provisional.api.TerminalState; @@ -24,29 +22,13 @@ public class SerialPortHandler private boolean isOpen; private boolean isPaused; private Object pauseMutex = new Object(); - private IProject project; - - private static final Map>> openPorts = new HashMap<>(); + private static final Map> openPorts = new ConcurrentHashMap<>(); private Process process; private Thread thread; private SerialConnector serialConnector; private SocketServerMessageHandler serverMessageHandler; - private SocketServerHandler socketServerHandler; - - private static String adjustPortName(String portName) - { - if (System.getProperty("os.name").startsWith("Windows") && !portName.startsWith("\\\\.\\")) //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ - { - return portName; // $NON-NLS-1$ - } - else - { - return portName; - } - } - /** * Return an the SerialPortHandler with the given name or null if it hasn't been allocated yet. This would be used * by components that need to pause and resume a serial port. @@ -57,19 +39,18 @@ private static String adjustPortName(String portName) */ public static SerialPortHandler get(String portName) { - synchronized (openPorts) + LinkedList list = openPorts.get(portName); + if (list == null) { - LinkedList> list = openPorts.get(adjustPortName(portName)); - if (list == null) - { - return null; - } + return null; + } - Iterator> i = list.iterator(); + synchronized (list) + { + Iterator i = list.iterator(); while (i.hasNext()) { - WeakReference ref = i.next(); - SerialPortHandler port = ref.get(); + SerialPortHandler port = i.next(); if (port == null) { i.remove(); @@ -79,16 +60,14 @@ public static SerialPortHandler get(String portName) return port; } } - - return null; } + return null; } public SerialPortHandler(String portName, SerialConnector serialConnector, IProject project) { - this.portName = adjustPortName(portName); + this.portName = portName; this.serialConnector = serialConnector; - this.project = project; this.serverMessageHandler = new SocketServerMessageHandler(serialConnector, project); } @@ -99,6 +78,7 @@ public String getPortName() private int startSocketServerThread() throws Exception { + SocketServerHandler socketServerHandler; socketServerHandler = SocketServerHandler.getInstance(); socketServerHandler.startServer(); @@ -126,34 +106,28 @@ public synchronized void open() serialConnector.filterOptions, serverPort); process = serialMonitorHandler.invokeIDFMonitor(); serialConnector.process = process; - thread = new Thread() - { - @Override - public void run() + thread = new Thread(() -> { + try (InputStream targetIn = process.getInputStream()) { - InputStream targetIn = process.getInputStream(); byte[] buff = new byte[256]; int n; - try + while ((n = targetIn.read(buff)) >= 0) { - while ((n = targetIn.read(buff, 0, buff.length)) >= 0) + if (n != 0) { - if (n != 0) - { - serialConnector.control.getRemoteToTerminalOutputStream().write(buff, 0, n); - } + serialConnector.control.getRemoteToTerminalOutputStream().write(buff, 0, n); } - - serialConnector.disconnect(); - } - catch (Exception e) - { - Activator.log(e); - serialConnector.disconnect(); - serialConnector.control.setState(TerminalState.CLOSED); } } - }; + catch (Exception e) + { + Activator.log(e); + } finally + { + serialConnector.disconnect(); + serialConnector.control.setState(TerminalState.CLOSED); + } + }); thread.start(); if (!serverMessageHandler.isAlive()) @@ -164,20 +138,10 @@ public void run() isOpen = true; serialConnector.control.setState(TerminalState.CONNECTED); - - synchronized (openPorts) - { - LinkedList> list = openPorts.get(portName); - if (list == null) - { - list = new LinkedList<>(); - openPorts.put(portName, list); - } - list.addFirst(new WeakReference<>(this)); - } + openPorts.computeIfAbsent(portName, k -> new LinkedList<>()).addFirst(this); } - public synchronized void close() throws IOException + public synchronized void close() { if (isOpen) { @@ -193,23 +157,10 @@ public synchronized void close() throws IOException thread.interrupt(); } - synchronized (openPorts) - { - LinkedList> list = openPorts.get(portName); - if (list != null) - { - Iterator> i = list.iterator(); - while (i.hasNext()) - { - WeakReference ref = i.next(); - SerialPortHandler port = ref.get(); - if (port == null || port.equals(this)) - { - i.remove(); - } - } - } - } + openPorts.computeIfPresent(portName, (k, list) -> { + list.remove(this); + return list.isEmpty() ? null : list; + }); try { @@ -228,7 +179,7 @@ public boolean isOpen() return isOpen; } - public void pause() throws IOException + public void pause() { if (!isOpen) { @@ -250,7 +201,7 @@ public void pause() throws IOException } } - public void resume() throws IOException + public void resume() { synchronized (pauseMutex) { diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java index fdb5e48af..088a7eaa4 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/launcher/SerialLauncherDelegate.java @@ -30,25 +30,30 @@ import com.espressif.idf.terminal.connector.serial.connector.SerialSettings; import com.espressif.idf.terminal.connector.serial.controls.SerialConfigPanel; -public class SerialLauncherDelegate extends AbstractLauncherDelegate { +public class SerialLauncherDelegate extends AbstractLauncherDelegate +{ @Override - public boolean needsUserConfiguration() { + public boolean needsUserConfiguration() + { return true; } @Override - public IConfigurationPanel getPanel(IConfigurationPanelContainer container) { + public IConfigurationPanel getPanel(IConfigurationPanelContainer container) + { return new SerialConfigPanel(container); } @Override - public ITerminalConnector createTerminalConnector(Map properties) { + public ITerminalConnector createTerminalConnector(Map properties) + { Assert.isNotNull(properties); // Check for the terminal connector id String connectorId = (String) properties.get(ITerminalsConnectorConstants.PROP_TERMINAL_CONNECTOR_ID); - if (connectorId == null) { + if (connectorId == null) + { connectorId = "com.espressif.idf.terminal.connector.serial.SerialConnector"; //$NON-NLS-1$ } @@ -63,7 +68,8 @@ public ITerminalConnector createTerminalConnector(Map properties // Construct the terminal connector instance ITerminalConnector connector = TerminalConnectorExtension.makeTerminalConnector(connectorId); - if (connector != null) { + if (connector != null) + { // Apply default settings connector.setDefaultSettings(); // And load the real settings @@ -74,7 +80,8 @@ public ITerminalConnector createTerminalConnector(Map properties } @Override - public void execute(Map properties, Done done) { + public void execute(Map properties, Done done) + { Assert.isNotNull(properties); // Set the terminal tab title @@ -83,14 +90,17 @@ public void execute(Map properties, Done done) { // Force a new terminal tab each time it is launched, if not set otherwise from outside // TODO need a command shell service routing to get this - if (!properties.containsKey(ITerminalsConnectorConstants.PROP_FORCE_NEW)) { + if (!properties.containsKey(ITerminalsConnectorConstants.PROP_FORCE_NEW)) + { properties.put(ITerminalsConnectorConstants.PROP_FORCE_NEW, Boolean.TRUE); } // Get the terminal service ITerminalService terminal = TerminalServiceFactory.getService(); // If not available, we cannot fulfill this request - if (terminal != null) { + if (terminal != null) + { + terminal.closeConsole(properties, done); terminal.openConsole(properties, done); } } From 4aaa6939f61d1007d328eb7b9d227ab42b9039fe Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Tue, 25 Mar 2025 10:39:04 +0200 Subject: [PATCH 2/6] feat: destroy process in finally --- .../terminal/connector/serial/connector/SerialPortHandler.java | 1 + 1 file changed, 1 insertion(+) diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index 4efeb1de5..4214ab1df 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -125,6 +125,7 @@ public synchronized void open() } finally { serialConnector.disconnect(); + serialConnector.process.destroy(); serialConnector.control.setState(TerminalState.CLOSED); } }); From 73d6915d9997273b86f416caae5a6f61cd8ceef0 Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Tue, 25 Mar 2025 16:26:35 +0200 Subject: [PATCH 3/6] feat: try to destroy process forcibly --- .../terminal/connector/serial/connector/SerialPortHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index 4214ab1df..e48a26de3 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -125,7 +125,7 @@ public synchronized void open() } finally { serialConnector.disconnect(); - serialConnector.process.destroy(); + serialConnector.process.destroyForcibly(); serialConnector.control.setState(TerminalState.CLOSED); } }); From aa96b373e958fb30dce4f45b49ef5106a099a3d8 Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Thu, 27 Mar 2025 18:01:00 +0200 Subject: [PATCH 4/6] fix: destroying forcibly when reflashing --- .../terminal/connector/serial/connector/SerialPortHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index e48a26de3..7d25a5f81 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -151,7 +151,7 @@ public synchronized void close() // kill the port process and thread if (process != null) { - process.destroy(); + process.destroyForcibly(); } if (thread != null) { From a8a53a8518bfb48f0c344728c2a1be43b885500f Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Fri, 28 Mar 2025 11:41:28 +0200 Subject: [PATCH 5/6] fix: closing streams before destroying process --- .../serial/connector/SerialPortHandler.java | 83 +++++++++++++------ 1 file changed, 59 insertions(+), 24 deletions(-) diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index 7d25a5f81..7aed0143d 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -1,10 +1,12 @@ package com.espressif.idf.terminal.connector.serial.connector; +import java.io.IOException; import java.io.InputStream; import java.util.Iterator; import java.util.LinkedList; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import org.eclipse.core.resources.IProject; import org.eclipse.tm.internal.terminal.provisional.api.TerminalState; @@ -125,7 +127,9 @@ public synchronized void open() } finally { serialConnector.disconnect(); + closeStreams(serialConnector.process); serialConnector.process.destroyForcibly(); + waitForProcessTermination(serialConnector.process); serialConnector.control.setState(TerminalState.CLOSED); } }); @@ -144,34 +148,37 @@ public synchronized void open() public synchronized void close() { - if (isOpen) + if (!isOpen) { - isOpen = false; + return; + } + isOpen = false; - // kill the port process and thread - if (process != null) - { - process.destroyForcibly(); - } - if (thread != null) - { - thread.interrupt(); - } + if (process != null) + { + closeStreams(process); + process.destroyForcibly(); + waitForProcessTermination(process); - openPorts.computeIfPresent(portName, (k, list) -> { - list.remove(this); - return list.isEmpty() ? null : list; - }); + } - try - { - // Sleep for a second since some serial ports take a while to actually close - Thread.sleep(500); - } - catch (InterruptedException e) - { - // nothing to do - } + if (thread != null && thread.isAlive()) + { + thread.interrupt(); + } + + openPorts.computeIfPresent(portName, (k, list) -> { + list.remove(this); + return list.isEmpty() ? null : list; + }); + + try + { + Thread.sleep(500); + } + catch (InterruptedException e) + { + Thread.currentThread().interrupt(); } } @@ -217,4 +224,32 @@ public void resume() } } + private void waitForProcessTermination(Process process) + { + try + { + if (!process.waitFor(2, TimeUnit.SECONDS)) + { + Logger.log("Process did not terminate in time"); + } + } + catch (InterruptedException e) + { + Logger.log(e); + } + } + + private void closeStreams(Process process) + { + try + { + process.getInputStream().close(); + process.getOutputStream().close(); + process.getErrorStream().close(); + } + catch (IOException e) + { + Logger.log(e); + } + } } From ad9f295ff558941b6e715a82c1584b0a7b211abb Mon Sep 17 00:00:00 2001 From: Denys Almazov Date: Fri, 28 Mar 2025 19:48:57 +0200 Subject: [PATCH 6/6] fix: stop web socket server before destroying the process --- .../serial/connector/SerialPortHandler.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java index 7aed0143d..77e4f28c2 100644 --- a/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java +++ b/bundles/com.espressif.idf.terminal.connector.serial/src/com/espressif/idf/terminal/connector/serial/connector/SerialPortHandler.java @@ -1,6 +1,5 @@ package com.espressif.idf.terminal.connector.serial.connector; -import java.io.IOException; import java.io.InputStream; import java.util.Iterator; import java.util.LinkedList; @@ -127,7 +126,8 @@ public synchronized void open() } finally { serialConnector.disconnect(); - closeStreams(serialConnector.process); + stopWebSocketServer(serialConnector.process); + serialConnector.process.destroyForcibly(); waitForProcessTermination(serialConnector.process); serialConnector.control.setState(TerminalState.CLOSED); @@ -156,7 +156,7 @@ public synchronized void close() if (process != null) { - closeStreams(process); + stopWebSocketServer(process); process.destroyForcibly(); waitForProcessTermination(process); @@ -239,15 +239,13 @@ private void waitForProcessTermination(Process process) } } - private void closeStreams(Process process) + private void stopWebSocketServer(Process process) { try { - process.getInputStream().close(); - process.getOutputStream().close(); - process.getErrorStream().close(); + SocketServerHandler.getInstance().stopServer(); } - catch (IOException e) + catch (Exception e) { Logger.log(e); }