From 7b84c1edbd82156aab1dcdc8a9df70e43e745685 Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:06:57 -0500 Subject: [PATCH 1/5] Fix critical robustness issues in GenericInstrument base class - Fix _tcp_query infinite loop when connection is closed by remote host - Add max buffer size limits to _tcp_query and _custom_eol_readline - Add UnicodeDecodeError handling in _tcp_query - Replace __exit__ -> __del__ pattern with proper close() method - Add try/finally in disconnect_tcp and disconnect_usb to ensure cleanup - Wrap constructor identity query in try/except to close on failure - Add length check in _get_identity for malformed *IDN? responses Co-Authored-By: Claude Opus 4.6 --- lakeshore/generic_instrument.py | 67 +++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/lakeshore/generic_instrument.py b/lakeshore/generic_instrument.py index 7454f61..50e065d 100644 --- a/lakeshore/generic_instrument.py +++ b/lakeshore/generic_instrument.py @@ -132,7 +132,10 @@ def __init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, par self.option_card_serial = serial_string[1] self.model_number = idn_response[1] except InstrumentException: - print('Instrument found but unable to communicate. Please check interface settings on the instrument.') + self.close() + raise + except Exception: + self.close() raise # Check to make sure the serial number matches what was provided if connecting over TCP @@ -141,17 +144,27 @@ def __init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, par "serial number provided is " + serial_number + ", serial number found is " + self.serial_number) + def close(self): + """Close all open connections.""" + if getattr(self, 'device_serial', None) is not None: + try: + self.device_serial.close() + finally: + self.device_serial = None + if getattr(self, 'device_tcp', None) is not None: + try: + self.device_tcp.close() + finally: + self.device_tcp = None + def __del__(self): - if self.device_serial is not None: - self.device_serial.close() - if self.device_tcp is not None: - self.device_tcp.close() + self.close() def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): - self.__del__() + self.close() def write(self, command_string): """Alias of command. Send a command to the instrument. @@ -233,8 +246,10 @@ def connect_tcp(self, ip_address, tcp_port, timeout): def disconnect_tcp(self): """Disconnect the TCP connection.""" - self.device_tcp.close() - self.device_tcp = None + try: + self.device_tcp.close() + finally: + self.device_tcp = None def connect_usb(self, serial_number=None, com_port=None, baud_rate=None, data_bits=None, stop_bits=None, parity=None, timeout=None, handshaking=None, flow_control=None): @@ -274,8 +289,10 @@ def connect_usb(self, serial_number=None, com_port=None, baud_rate=None, data_bi def disconnect_usb(self): """Disconnect the USB connection.""" - self.device_serial.close() - self.device_serial = None + try: + self.device_serial.close() + finally: + self.device_serial = None def _tcp_command(self, command): """Send a command over the TCP connection.""" @@ -286,22 +303,28 @@ def _tcp_query(self, query): """Query over the TCP connection.""" self._tcp_command(query) - total_response = "" + MAX_BUFFER_SIZE = 1 * 1024 * 1024 # 1 MB - # Continuously receive data from the buffer until a line break while True: - - # Receive the data and raise an error on timeout try: - response = self.device_tcp.recv(4096).decode('utf-8') + raw_bytes = self.device_tcp.recv(4096) except socket.timeout as ex: raise InstrumentException("Connection timed out") from ex - # Add received information to the response + if not raw_bytes: + raise InstrumentException("Connection closed by remote host") + + try: + response = raw_bytes.decode('utf-8') + except UnicodeDecodeError as ex: + raise InstrumentException("Invalid response encoding") from ex + total_response += response - # Return the response once it ends with a line break + if len(total_response) > MAX_BUFFER_SIZE: + raise InstrumentException("Response exceeded maximum buffer size") + if total_response.endswith("\r\n"): return total_response.rstrip() @@ -323,12 +346,14 @@ def _usb_query(self, query): return response.rstrip() def _custom_eol_readline(self): + MAX_LINE_SIZE = 1 * 1024 * 1024 # 1 MB line = bytearray() while True: new_character = self.device_serial.read(1) if new_character: line += new_character - # Check to see if the last two characters are the terminator characters \r\n + if len(line) > MAX_LINE_SIZE: + raise InstrumentException("Serial response exceeded maximum buffer size") if line[-2:] == b'\r\n': break else: @@ -351,4 +376,8 @@ def _user_connection_query(self, query): return response def _get_identity(self): - return self.query('*IDN?').split(',') + idn_response = self.query('*IDN?').split(',') + if len(idn_response) < 4: + raise InstrumentException( + f"Malformed *IDN? response: expected at least 4 comma-separated fields, got {len(idn_response)}") + return idn_response From b59dfe7de15de76151175076d42df2510d4e315b Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:15:35 -0500 Subject: [PATCH 2/5] Remove dead enum34 dependency and clean up wildcard imports - Remove unreachable enum34 dependency from setup.py and docs-requirements.txt (python_requires='>=3.7' makes the python_version<'3.4' condition dead code) - Remove redundant explicit Model336 import already covered by wildcard import - Add stub notes to Model 350/425 modules and fix copy-paste comments referencing "121" Co-Authored-By: Claude Opus 4.6 --- docs-requirements.txt | 1 - lakeshore/__init__.py | 1 - lakeshore/model_350.py | 7 +++++-- lakeshore/model_425.py | 7 +++++-- setup.py | 1 - 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs-requirements.txt b/docs-requirements.txt index 3207cf6..f882495 100644 --- a/docs-requirements.txt +++ b/docs-requirements.txt @@ -3,5 +3,4 @@ sphinx_rtd_theme pyserial>=3.0 iso8601 packaging -enum34 wakepy>=0.7.1 \ No newline at end of file diff --git a/lakeshore/__init__.py b/lakeshore/__init__.py index 4cfb0ed..9ab65d5 100644 --- a/lakeshore/__init__.py +++ b/lakeshore/__init__.py @@ -12,7 +12,6 @@ from .model_240 import * from .model_335 import * from .model_336 import * -from .model_336 import Model336 from .model_350 import Model350 from .model_372 import * from .model_425 import Model425 diff --git a/lakeshore/model_350.py b/lakeshore/model_350.py index 124f01b..0b9690c 100644 --- a/lakeshore/model_350.py +++ b/lakeshore/model_350.py @@ -1,4 +1,7 @@ -"""Implements functionality unique to the Lake Shore Model 350 cryogenic temperature controller.""" +"""Implements functionality unique to the Lake Shore Model 350 cryogenic temperature controller. + +NOTE: This module is a non-functional stub. No instrument-specific methods have been implemented yet. +""" import serial from .generic_instrument import GenericInstrument @@ -23,6 +26,6 @@ def __init__(self, tcp_port=7777, **kwargs): - # Call the parent init, then fill in values specific to the 121 + # Call the parent init GenericInstrument.__init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, parity, flow_control, handshaking, timeout, ip_address, tcp_port, **kwargs) diff --git a/lakeshore/model_425.py b/lakeshore/model_425.py index 17b2469..9c68bff 100644 --- a/lakeshore/model_425.py +++ b/lakeshore/model_425.py @@ -1,4 +1,7 @@ -"""Implements functionality unique to the Lake Shore Model 425 Gaussmeter.""" +"""Implements functionality unique to the Lake Shore Model 425 Gaussmeter. + +NOTE: This module is a non-functional stub. No instrument-specific methods have been implemented yet. +""" import serial from .generic_instrument import GenericInstrument @@ -23,6 +26,6 @@ def __init__(self, tcp_port=7777, **kwargs): - # Call the parent init, then fill in values specific to the 121 + # Call the parent init GenericInstrument.__init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, parity, flow_control, handshaking, timeout, ip_address, tcp_port, **kwargs) diff --git a/setup.py b/setup.py index 14ec09c..678809f 100644 --- a/setup.py +++ b/setup.py @@ -23,7 +23,6 @@ install_requires=['pyserial>=3.0', 'iso8601', 'packaging', - "enum34;python_version<'3.4'", 'wakepy>=0.7.1'], classifiers=['Programming Language :: Python :: 3'] ) From c59729c803346e81f2016dc61deb4a38f3c4b9bf Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:24:00 -0500 Subject: [PATCH 3/5] Fix thread safety and remaining robustness issues in GenericInstrument - close() now acquires dut_lock to prevent races with concurrent command()/query() calls, and clears user_connection reference - Serial-number mismatch check moved inside try/except so close() is called on failure (previously leaked the TCP socket) - Collapsed redundant duplicate except blocks into single except Exception - __del__ wrapped in try/except to handle GC and interpreter shutdown - disconnect_tcp()/disconnect_usb() now acquire dut_lock and guard against being called when the connection is already None - _tcp_query catches OSError for non-timeout socket failures (ConnectionResetError, BrokenPipeError, etc.) Co-Authored-By: Claude Opus 4.6 --- lakeshore/generic_instrument.py | 82 +++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/lakeshore/generic_instrument.py b/lakeshore/generic_instrument.py index 50e065d..44f488b 100644 --- a/lakeshore/generic_instrument.py +++ b/lakeshore/generic_instrument.py @@ -131,34 +131,50 @@ def __init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, par if len(serial_string) == 2: self.option_card_serial = serial_string[1] self.model_number = idn_response[1] - except InstrumentException: - self.close() - raise + + # Check to make sure the serial number matches what was provided if connecting over TCP + if ip_address is not None and serial_number is not None and serial_number != self.serial_number: + raise InstrumentException("Instrument found but the serial number does not match. " + + "serial number provided is " + serial_number + + ", serial number found is " + self.serial_number) except Exception: self.close() raise - # Check to make sure the serial number matches what was provided if connecting over TCP - if ip_address is not None and serial_number is not None and serial_number != self.serial_number: - raise InstrumentException("Instrument found but the serial number does not match. " + - "serial number provided is " + serial_number + - ", serial number found is " + self.serial_number) - def close(self): - """Close all open connections.""" - if getattr(self, 'device_serial', None) is not None: - try: - self.device_serial.close() - finally: - self.device_serial = None - if getattr(self, 'device_tcp', None) is not None: - try: - self.device_tcp.close() - finally: - self.device_tcp = None + """Close all open connections. + + Caller-provided connections (via the connection parameter) are not closed, + as their lifecycle is owned by the caller. The reference is cleared. + + Safe to call multiple times. Acquires dut_lock to prevent races with + concurrent command()/query() calls. + """ + lock = getattr(self, 'dut_lock', None) + if lock is not None: + lock.acquire() + try: + if getattr(self, 'device_serial', None) is not None: + try: + self.device_serial.close() + finally: + self.device_serial = None + if getattr(self, 'device_tcp', None) is not None: + try: + self.device_tcp.close() + finally: + self.device_tcp = None + if getattr(self, 'user_connection', None) is not None: + self.user_connection = None + finally: + if lock is not None: + lock.release() def __del__(self): - self.close() + try: + self.close() + except Exception: + pass def __enter__(self): return self @@ -246,10 +262,13 @@ def connect_tcp(self, ip_address, tcp_port, timeout): def disconnect_tcp(self): """Disconnect the TCP connection.""" - try: - self.device_tcp.close() - finally: - self.device_tcp = None + with self.dut_lock: + if self.device_tcp is None: + return + try: + self.device_tcp.close() + finally: + self.device_tcp = None def connect_usb(self, serial_number=None, com_port=None, baud_rate=None, data_bits=None, stop_bits=None, parity=None, timeout=None, handshaking=None, flow_control=None): @@ -289,10 +308,13 @@ def connect_usb(self, serial_number=None, com_port=None, baud_rate=None, data_bi def disconnect_usb(self): """Disconnect the USB connection.""" - try: - self.device_serial.close() - finally: - self.device_serial = None + with self.dut_lock: + if self.device_serial is None: + return + try: + self.device_serial.close() + finally: + self.device_serial = None def _tcp_command(self, command): """Send a command over the TCP connection.""" @@ -311,6 +333,8 @@ def _tcp_query(self, query): raw_bytes = self.device_tcp.recv(4096) except socket.timeout as ex: raise InstrumentException("Connection timed out") from ex + except OSError as ex: + raise InstrumentException(f"TCP communication error: {ex}") from ex if not raw_bytes: raise InstrumentException("Connection closed by remote host") From 93467ce422566b2bcfb1dba265eade64219cd9ab Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:24:35 -0500 Subject: [PATCH 4/5] Add IDN response validation to subclass _get_identity overrides The base GenericInstrument._get_identity() validates that the *IDN? response has at least 4 comma-separated fields, but subclass overrides in XIPInstrument, TemperatureController, and Model224 returned the raw split result without validation. A malformed response would cause an IndexError in the constructor instead of a clear InstrumentException. Also fix missing trailing newline in docs-requirements.txt. Co-Authored-By: Claude Opus 4.6 --- docs-requirements.txt | 2 +- lakeshore/model_224.py | 6 +++++- lakeshore/temperature_controllers.py | 6 +++++- lakeshore/xip_instrument.py | 6 +++++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs-requirements.txt b/docs-requirements.txt index f882495..1b41185 100644 --- a/docs-requirements.txt +++ b/docs-requirements.txt @@ -3,4 +3,4 @@ sphinx_rtd_theme pyserial>=3.0 iso8601 packaging -wakepy>=0.7.1 \ No newline at end of file +wakepy>=0.7.1 diff --git a/lakeshore/model_224.py b/lakeshore/model_224.py index 8c3a2b8..2e2e64d 100644 --- a/lakeshore/model_224.py +++ b/lakeshore/model_224.py @@ -1213,7 +1213,11 @@ def get_relay_control_mode(self, relay_number): return self.RelayControlMode(int(split_relay_settings[0])) def _get_identity(self): - return self.query('*IDN?', check_errors=False).split(',') + idn_response = self.query('*IDN?', check_errors=False).split(',') + if len(idn_response) < 4: + raise InstrumentException( + f"Malformed *IDN? response: expected at least 4 comma-separated fields, got {len(idn_response)}") + return idn_response __all__ = ['Model224', 'Model224AlarmParameters', 'Model224CurveHeader', 'Model224StandardEventRegister', diff --git a/lakeshore/temperature_controllers.py b/lakeshore/temperature_controllers.py index b1a5c3d..2b3dff8 100644 --- a/lakeshore/temperature_controllers.py +++ b/lakeshore/temperature_controllers.py @@ -1405,7 +1405,11 @@ def _get_website_login(self): "password": login_response[1]} def _get_identity(self): - return self.query('*IDN?', check_errors=False).split(',') + idn_response = self.query('*IDN?', check_errors=False).split(',') + if len(idn_response) < 4: + raise InstrumentException( + f"Malformed *IDN? response: expected at least 4 comma-separated fields, got {len(idn_response)}") + return idn_response def _autotune_error(self): """Method to raise an exception if autotune error has occurred.""" diff --git a/lakeshore/xip_instrument.py b/lakeshore/xip_instrument.py index 661014a..d103053 100644 --- a/lakeshore/xip_instrument.py +++ b/lakeshore/xip_instrument.py @@ -408,4 +408,8 @@ def factory_reset(self): self.command("SYSTEM:FACTORYRESET") def _get_identity(self): - return self.query('*IDN?', check_errors=False).split(',') + idn_response = self.query('*IDN?', check_errors=False).split(',') + if len(idn_response) < 4: + raise InstrumentException( + f"Malformed *IDN? response: expected at least 4 comma-separated fields, got {len(idn_response)}") + return idn_response From 8d342e776247e50b20b6d57e25368fd43387e78b Mon Sep 17 00:00:00 2001 From: Justin Fichtner Date: Tue, 17 Feb 2026 19:31:42 -0500 Subject: [PATCH 5/5] Wrap serial read errors and promote buffer constants to class-level - _custom_eol_readline now catches OSError from serial read() and wraps it in InstrumentException, consistent with _tcp_query's OSError handling - Moved MAX_BUFFER_SIZE from local variables in _tcp_query and _custom_eol_readline to a single class-level constant, making it discoverable and overridable by subclasses Co-Authored-By: Claude Opus 4.6 --- lakeshore/generic_instrument.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lakeshore/generic_instrument.py b/lakeshore/generic_instrument.py index 44f488b..4a1015d 100644 --- a/lakeshore/generic_instrument.py +++ b/lakeshore/generic_instrument.py @@ -84,6 +84,7 @@ class GenericInstrument: vid_pid = [] logger = logging.getLogger(__name__) + MAX_BUFFER_SIZE = 1 * 1024 * 1024 # 1 MB def __init__(self, serial_number, com_port, baud_rate, data_bits, stop_bits, parity, flow_control, handshaking, timeout, ip_address, tcp_port, connection=None): @@ -326,7 +327,6 @@ def _tcp_query(self, query): self._tcp_command(query) total_response = "" - MAX_BUFFER_SIZE = 1 * 1024 * 1024 # 1 MB while True: try: @@ -346,7 +346,7 @@ def _tcp_query(self, query): total_response += response - if len(total_response) > MAX_BUFFER_SIZE: + if len(total_response) > self.MAX_BUFFER_SIZE: raise InstrumentException("Response exceeded maximum buffer size") if total_response.endswith("\r\n"): @@ -370,13 +370,15 @@ def _usb_query(self, query): return response.rstrip() def _custom_eol_readline(self): - MAX_LINE_SIZE = 1 * 1024 * 1024 # 1 MB line = bytearray() while True: - new_character = self.device_serial.read(1) + try: + new_character = self.device_serial.read(1) + except OSError as ex: + raise InstrumentException(f"Serial communication error: {ex}") from ex if new_character: line += new_character - if len(line) > MAX_LINE_SIZE: + if len(line) > self.MAX_BUFFER_SIZE: raise InstrumentException("Serial response exceeded maximum buffer size") if line[-2:] == b'\r\n': break