[fix] hold the Machine lock for the whole REPL session#174
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 7 |
| Duplication | 2 |
🟢 Coverage 100.00% diff coverage · +0.03% coverage variation
Metric Results Coverage variation ✅ +0.03% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (d97f2a1) 7874 7458 94.72% Head commit (468b610) 7915 (+41) 7499 (+41) 94.74% (+0.03%) 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 (#174) 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%
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 #174 +/- ##
==========================================
+ Coverage 93.52% 93.54% +0.01%
==========================================
Files 50 50
Lines 6298 6316 +18
==========================================
+ Hits 5890 5908 +18
Misses 259 259
Partials 149 149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8d3f1ff to
3c20696
Compare
REPL() drove prepareThread and repl.REPLOptions against m.thread and m.predeclared with no lock, while every other execution path (Run, RunFile, RunWithTimeout, ...) holds m.mu for the whole run and Reset holds it to swap the thread. A REPL session concurrent with a Run or Reset on the same Machine was therefore a data race on m.thread. REPL now takes the write lock for its whole session, matching runInternal; String() already uses TryRLock and returns the running snapshot rather than blocking, so it stays responsive during a REPL. TestMachine_REPLIsRaceFree drives the REPL non-interactively (stdin pointed at /dev/null so repl.REPLOptions returns on EOF) and runs it concurrently with Run and Reset; it reports a data race under -race before this change and is clean after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3c20696 to
468b610
Compare
Problem
REPL()droveprepareThreadandrepl.REPLOptionsagainstm.threadandm.predeclaredwith no lock, while every other execution path (Run,RunFile,RunWithTimeout,RunWithContext, …) holdsm.mufor the whole run andResetholds it to swap the thread (STAR-69). A REPL session concurrent with aRunorReseton the sameMachinewas therefore a data race onm.thread.Fix
REPL()now takesm.mu.Lock()/defer Unlock()for its whole session, matchingrunInternal. A REPL owns the Machine exclusively for its duration, so holding the lock until it exits is the right scope.String()already usesTryRLockand returns the running snapshot rather than blocking, so it stays responsive during a REPL.prepareThreadandgetFileOptionsdo not re-acquirem.mu, so there is no self-deadlock.Test
TestMachine_REPLIsRaceFreedrives the REPL non-interactively (stdin pointed atos.DevNull, sorepl.REPLOptionsreturns on EOF) and runs it concurrently withRunandReset. It reports a data race under-racebefore this change and is clean after.Verification
gofmt/go vetclean;go test,go test -race -count=2, full./..., anddocker golang:1.19 go test -raceall green.Part of the STAR-69 follow-up (residual flagged during that fix's review).