feat(lib): transport base class that implements shared ownership of transport - #336
Conversation
|
Claude finished @wharwood's task in 3m 5s —— View job Review complete
Overview
Two inline nits posted:
No breaking-change concerns: existing non-shared callers never populate |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Greptile SummaryThe PR introduces
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported reentrant callback issue is fixed by retaining the session whenever the callback registers a new holder. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self._teardown_session() | ||
|
|
||
| def close(self) -> None: |
There was a problem hiding this comment.
When on_last_release directly or indirectly calls acquire() on the same transport, the reentrant lock allows a new holder to be registered before the finally block unconditionally closes its session, leaving that holder with stale ownership and failing subsequent I/O.
Prompt To Fix With AI
This is a comment left during a code review.
Path: instro/lib/transports/ownership.py
Line: 47-49
Comment:
**Reentrant callback invalidates ownership**
When `on_last_release` directly or indirectly calls `acquire()` on the same transport, the reentrant lock allows a new holder to be registered before the `finally` block unconditionally closes its session, leaving that holder with stale ownership and failing subsequent I/O.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def lock(self) -> threading.RLock: | ||
| """Return the reentrant resource lock for atomic multi-step sequences. | ||
|
|
||
| Example:: | ||
|
|
||
| with driver.lock(): | ||
| driver.write("CONF:VOLT:DC") | ||
| driver.write("RANGE 10") | ||
| value = driver.query("READ?") | ||
|
|
||
| Reentrant: the holding thread can call write/query/read inside the with. | ||
| """ |
There was a problem hiding this comment.
Multi-line docstring (with an Example:: block) landing in a brand-new file — AGENTS.md's one-short-line-max rule (INSTRO-248) applies here even though the content was relocated from VisaDriver.lock(). Worth trimming to one line while it's moved.
| def open(self) -> None: | ||
| if self._visa.acquire(self): # True only for the first owner | ||
| self._visa.write("SYST:LOCK ON") # one-time device setup |
There was a problem hiding this comment.
This worked example's open() doesn't show the release-before-raise pattern that the new AGENTS.md bullet ("A driver whose post-acquire device setup raises must release(self) before propagating") mandates — tests/lib/test_shared_transport.py's _SharedPSUDriver wraps the write in try/except and releases on failure, but this doc snippet doesn't. Readers copying this example would miss the stranded-holder pitfall.
wharwood
changed the title
2 participants
Summary
Adds
OwnershipContexTransportBase, a new base class in instro/lib/transports/ownership.pytransport_base.py that lets multiple drivers share one VisaDriver or ModbusDriver connection via open()/close(): the first open() opens the connection and reports first owner status so one-time device setup (e.g. taking a combined instrument's remote lock) runs exactly once, and the connection stays open until the last close() frees it, running an optional teardown callback in the right order before the socket closes. Both VisaDriver and ModbusDriver now inherit this base instead of duplicating lock/lifecycle bookkeeping, with a guarded public close() that declines (and logs) while any owner remains and a del GC backstop that bypasses the guard to avoid stranding a resource. This unblocks combined instruments like the EA PSB 10000 series, where a PSU-shaped driver and an ELoad-shaped driver need to share one physical connection without either surface prematurely closing it out from under the other. Closes INSTRO-512.Type of change
fix)feat)feat!/fix!)refactor)docs)chore)Verification
This was tested with the added unit tests and mock transports.
Tests
Checklist
feat(driver): add support for Keysight E36300)Notes for reviewers
This is the bases for drivers that will share a transport but implement more than one base instrument. It should be thoroughly discussed prior to merge.