[fix] make Machine.String race-free without blocking, and lock Reset - #173
Conversation
Machine.String() read m.thread, m.runTimes, m.scriptName and m.scriptContent with no lock while Run and Reset write exactly those fields. Every other accessor already takes the read lock, so this was an omission, not a trade-off: a host printing the machine from one goroutine while another ran it is a data race (-race reports it), and the separate nil-check-then-deref of m.thread let a concurrent Reset null it between and panic the host. String() now guards the read, but with TryRLock rather than RLock so it never blocks: Run holds the write lock for the whole execution, so a plain RLock would self-deadlock when String is reached from within a callback of that same run — `fmt`-logging the machine inside its own print func is an easy way to hit it. When the lock is unavailable a run/reset is in flight, so String reports the machine as running rather than reading fields under mutation. Reset() took no lock while clearing thread/runTimes/predeclared — the fields the readers read — so locking only the readers would not have removed the race. It now takes the write lock like the other mutators. Not addressed here (pre-existing, separate follow-up): Machine.REPL() writes thread/predeclared without the lock, so String/Reset still race with a running REPL; giving REPL a locking strategy that does not block accessors for a whole interactive session is its own change. Tests: TestMachine_StringIsRaceFree (Run+Reset+String concurrently; the race detector reported 8 races before) and TestMachine_StringInCallbackDoesNotDeadlock (String from the print callback returns the running snapshot instead of hanging). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
| Duplication | 0 |
🟢 Coverage 100.00% diff coverage · +0.02% coverage variation
Metric Results Coverage variation ✅ +0.02% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (5d4c419) Report Missing Report Missing Report Missing Head commit (3b5da3b) 7781 (+8) 7365 (+9) 94.65% (+0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#173) 10 10 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 93.42% 93.44% +0.02%
==========================================
Files 50 50
Lines 6222 6227 +5
==========================================
+ Hits 5813 5819 +6
+ Misses 261 259 -2
- Partials 148 149 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What
Machine.String()readm.thread,m.runTimes,m.scriptName,m.scriptContentwith no lock whileRunandResetwrite exactly those fields. Every other accessor already takes the read lock, so this was an omission, not a trade-off: printing the machine from one goroutine while another runs it is a data race (-racereports it), and the separate nil-check-then-deref ofm.threadlet a concurrentResetnull it between and panic the host.String()now guards the read withTryRLockrather thanRLock, so it never blocks:Runholds the write lock for the whole execution, so a plainRLockwould self-deadlock whenStringis reached from within a callback of that same run (fmt-logging the machine inside its own print func is an easy way to hit it). When the lock is unavailable a run/reset/setter is in flight, soStringreports the machine as running rather than reading fields under mutation.Reset()took no lock while clearingthread/runTimes/predeclared— the fields the readers read — so locking only the readers would not have removed the race. It now takes the write lock like the other mutators.Not addressed here (pre-existing follow-up)
Machine.REPL()writesthread/predeclaredwithout the lock, soString/Resetstill race with a running REPL; a locking strategy for an interactive session that does not block accessors is its own change.Tests
Run+Reset+String concurrently (the race detector reported 8 races before);
Stringfrom the print callback returns the running snapshot instead of hanging. Full-race/vet/gofmt/Docker go1.19 clean.