Skip to content

Fix data race / nil-pointer panic between serve goroutine and Close - #13

Merged
MeteorSis merged 1 commit into
daangn:mainfrom
reinkrul:fix-accept-goroutine-race
Jun 2, 2026
Merged

Fix data race / nil-pointer panic between serve goroutine and Close#13
MeteorSis merged 1 commit into
daangn:mainfrom
reinkrul:fix-accept-goroutine-race

Conversation

@reinkrul

@reinkrul reinkrul commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Problem

Closing a server shortly after Run() can panic in a goroutine that no caller can recover, crashing the test binary:

WARNING: DATA RACE
panic: runtime error: invalid memory address or nil pointer dereference
  github.com/daangn/minimemcached.(*MiniMemcached).serve()  minimemcached.go:125

Root cause

Run()start() sets m.server = s and spawns go m.serve(). The goroutine loops on m.l.Accept(), reading the embedded listener field on every iteration:

func (m *MiniMemcached) serve() {
	for {
		conn, err := m.l.Accept()   // reads m.l
		...
	}
}

Close()server.close() concurrently nils that same field:

func (s *server) close() {
	if s.l != nil {
		_ = s.l.Close()
		s.l = nil               // writes m.l
	}
}

Two defects:

  • Data race: serve() reads m.l with no lock while Close() writes it under m.mu.
  • Nil dereference: if Close() runs before the freshly spawned serve goroutine first reads m.l (common for short-lived servers in unit tests), Accept() is called on a nil listener and panics — in a goroutine the caller cannot recover.

Fix

Capture the listener synchronously in newServer(), which executes during Run() and therefore happens-before any Close() (a caller can only Close() after Run() returns). Pass it into serve() so the goroutine never touches the shared field. Close() still closes the same listener, so the parked Accept() unblocks with an error and serve() returns cleanly.

Test

Adds TestRunClose, which loops Run()/Close(). Under go test -race it reliably reproduces both the data race and the nil-pointer panic on the current code, and passes with the fix.

serve() read the m.l listener field on every Accept() loop iteration,
while Close() -> server.close() concurrently set that field to nil. When
Close() ran before the freshly spawned serve goroutine first read m.l
(common on short-lived servers), Accept() was invoked on a nil listener,
panicking in a goroutine no caller can recover.

Capture the listener synchronously in newServer() - which runs during
Run(), happens-before any Close() - and pass it into serve(), so the
goroutine never touches the shared field. Close() still closes the same
listener, unblocking the parked Accept() cleanly.

Adds TestRunClose, which reproduces both the data race and the panic
under -race on the old code.

@MeteorSis MeteorSis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution.

@MeteorSis
MeteorSis merged commit 3472169 into daangn:main Jun 2, 2026
1 check passed
reinkrul added a commit to nuts-foundation/nuts-node that referenced this pull request Jun 4, 2026
v1.2.1 includes the upstream fix (daangn/minimemcached#13) for the
nil-pointer panic between the serve goroutine and Close, so the probe
loop that waited for the accept loop to park is no longer needed.

Assisted by AI
reinkrul added a commit to nuts-foundation/nuts-node that referenced this pull request Jun 4, 2026
…#4326)

* chore(deps): bump github.com/daangn/minimemcached from 1.2.0 to 1.2.1

Bumps [github.com/daangn/minimemcached](https://github.com/daangn/minimemcached) from 1.2.0 to 1.2.1.
- [Release notes](https://github.com/daangn/minimemcached/releases)
- [Commits](daangn/minimemcached@v1.2.0...v1.2.1)

---
updated-dependencies:
- dependency-name: github.com/daangn/minimemcached
  dependency-version: 1.2.1
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* test(storage): remove minimemcached startup-race workaround

v1.2.1 includes the upstream fix (daangn/minimemcached#13) for the
nil-pointer panic between the serve goroutine and Close, so the probe
loop that waited for the accept loop to park is no longer needed.

Assisted by AI

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rein Krul <info@reinkrul.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants