Fix GenericInstrument robustness and clean up dead code#14
Open
jfichtner wants to merge 5 commits into
Open
Conversation
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- _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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR significantly improves the robustness and maintainability of the Lake Shore Python driver by addressing critical connection management issues, thread safety concerns, and error handling in the base GenericInstrument class, while also cleaning up dead code and fixing documentation errors.
Changes:
- Fixed critical robustness issues in GenericInstrument: infinite loop on TCP connection close, race conditions, double-close bugs, and resource leaks
- Added input validation for
*IDN?responses across all instrument classes to prevent downstream parsing errors - Removed dead
enum34dependency (incompatible withpython_requires='>=3.7') and redundant imports
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Removed unreachable enum34 dependency for Python <3.4 |
| docs-requirements.txt | Removed enum34 dependency |
| lakeshore/init.py | Removed redundant explicit Model336 import (already covered by wildcard) |
| lakeshore/model_350.py | Added stub documentation note and fixed copy-paste comment |
| lakeshore/model_425.py | Added stub documentation note and fixed copy-paste comment |
| lakeshore/generic_instrument.py | Comprehensive robustness fixes: new close() method with thread safety, fixed infinite loop on TCP close, added buffer size limits, improved error handling, fixed resource leak on constructor failure, added IDN validation |
| lakeshore/xip_instrument.py | Added IDN response validation in _get_identity() |
| lakeshore/temperature_controllers.py | Added IDN response validation in _get_identity() |
| lakeshore/model_224.py | Added IDN response validation in _get_identity() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
__exit__/__del__, validate*IDN?responses, close serial port on constructor failure, add max buffer sizes, use try/finally in disconnect methodsclose()anddisconnect_*()now acquiredut_lockto prevent races with concurrentcommand()/query()calls;__del__wrapped in try/except for GC/interpreter shutdown safetyclose()is called before re-raising_tcp_queryand_custom_eol_readlinenow catchOSErrorand wrap inInstrumentException_get_identity()overrides (XIPInstrument, TemperatureController, Model224)enum34dependency from setup.py and docs-requirements.txt (unreachable sincepython_requires='>=3.7')Model336import already covered by wildcardTest plan
python -m pytest tests/ -v— all 866 tests pass after each commit🤖 Generated with Claude Code