fix: release player resources on close

commit: 8a753799e41c9e0f316cd74fd41382746650ec50
author: brice · 6mo ago · 2026-04-10 03:53 UTC
parent: 0db6373
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

4 files · +173 -18

M internal/capi/stream_ops.go +4 -8


@@ -27,9 +27,8 @@ type streamCallbackStorage struct {
27	27		// streamOpsImpl implements portout.StreamOps using the real PipeWire C API
28	28		// bindings registered via purego.
29	29		type streamOpsImpl struct {
30		-	mu sync.Mutex
31		-	pinned map[unsafe.Pointer]*streamCallbackStorage // keyed by stream ptr
32		-	destroyed map[unsafe.Pointer]bool // guard double-destroy
	30	+	mu sync.Mutex
	31	+	pinned map[unsafe.Pointer]*streamCallbackStorage // keyed by stream ptr
33	32		}
34	33
35	34		// Verify interface compliance at compile time.

@@ -106,14 +105,11 @@ func (s *streamOpsImpl) DisconnectStream(streamPtr unsafe.Pointer) {
106	105
107	106		func (s *streamOpsImpl) DestroyStream(streamPtr unsafe.Pointer) {
108	107		s.mu.Lock()
109		-	if s.destroyed == nil {
110		-	s.destroyed = make(map[unsafe.Pointer]bool)
111		-	}
112		-	if s.destroyed[streamPtr] {
	108	+	if _, alive := s.pinned[streamPtr]; !alive {
	109	+	// Already destroyed or never tracked — skip.
113	110		s.mu.Unlock()
114	111		return
115	112		}
116		-	s.destroyed[streamPtr] = true
117	113		// Unpin callback storage — safe now that the stream is being destroyed.
118	114		delete(s.pinned, streamPtr)
119	115		s.mu.Unlock()

 // streamOpsImpl implements portout.StreamOps using the real PipeWire C API
 // bindings registered via purego.
 type streamOpsImpl struct {
-	mu        sync.Mutex
+	mu     sync.Mutex
-	pinned    map[unsafe.Pointer]*streamCallbackStorage // keyed by stream ptr
+	pinned map[unsafe.Pointer]*streamCallbackStorage // keyed by stream ptr
-	destroyed map[unsafe.Pointer]bool                   // guard double-destroy
 }
 // Verify interface compliance at compile time.
 func (s *streamOpsImpl) DestroyStream(streamPtr unsafe.Pointer) {
 	s.mu.Lock()
-	if s.destroyed == nil {
+	if _, alive := s.pinned[streamPtr]; !alive {
-		s.destroyed = make(map[unsafe.Pointer]bool)
+		// Already destroyed or never tracked — skip.
-	}
-	if s.destroyed[streamPtr] {
 		s.mu.Unlock()
 		return
 	}
-	s.destroyed[streamPtr] = true
 	// Unpin callback storage — safe now that the stream is being destroyed.
 	delete(s.pinned, streamPtr)
 	s.mu.Unlock()

M internal/capi/stream_ops_test.go +28 -8


@@ -20,8 +20,7 @@ func TestDestroyStreamCallsPwStreamDestroy(t *testing.T) {
20	20		}
21	21
22	22		ops := &streamOpsImpl{
23		-	pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
24		-	destroyed: make(map[unsafe.Pointer]bool),
	23	+	pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
25	24		}
26	25
27	26		fakePtr := unsafe.Pointer(uintptr(0x1234))

@@ -36,10 +35,7 @@ func TestDestroyStreamCallsPwStreamDestroy(t *testing.T) {
36	35		t.Fatalf("pw_stream_destroy received %v, want %v", receivedPtr, fakePtr)
37	36		}
38	37
39		-	// Verify internal bookkeeping.
40		-	if !ops.destroyed[fakePtr] {
41		-	t.Error("expected stream to be marked as destroyed")
42		-	}
	38	+	// Verify internal bookkeeping: pinned entry should be removed.
43	39		if _, exists := ops.pinned[fakePtr]; exists {
44	40		t.Error("expected callback storage to be unpinned after destroy")
45	41		}

@@ -57,8 +53,7 @@ func TestDestroyStreamDoubleDestroySkipsSecondCall(t *testing.T) {
57	53		}
58	54
59	55		ops := &streamOpsImpl{
60		-	pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
61		-	destroyed: make(map[unsafe.Pointer]bool),
	56	+	pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
62	57		}
63	58
64	59		fakePtr := unsafe.Pointer(uintptr(0xABCD))

@@ -72,6 +67,31 @@ func TestDestroyStreamDoubleDestroySkipsSecondCall(t *testing.T) {
72	67		}
73	68		}
74	69
	70	+	// TestDestroyStreamBookkeepingDoesNotLeak verifies that internal tracking
	71	+	// does not retain entries after cleanup across many create/destroy cycles.
	72	+	func TestDestroyStreamBookkeepingDoesNotLeak(t *testing.T) {
	73	+	origDestroy := pw_stream_destroy
	74	+	t.Cleanup(func() { pw_stream_destroy = origDestroy })
	75	+
	76	+	pw_stream_destroy = func(_ unsafe.Pointer) {}
	77	+
	78	+	ops := &streamOpsImpl{
	79	+	pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
	80	+	}
	81	+
	82	+	const cycles = 100
	83	+	for i := uintptr(1); i <= cycles; i++ {
	84	+	ptr := unsafe.Pointer(i)
	85	+	ops.pinned[ptr] = &streamCallbackStorage{}
	86	+	ops.DestroyStream(ptr)
	87	+	}
	88	+
	89	+	// After destroying all streams, pinned map should be empty.
	90	+	if len(ops.pinned) != 0 {
	91	+	t.Fatalf("pinned map has %d entries after %d cycles; expected 0", len(ops.pinned), cycles)
	92	+	}
	93	+	}
	94	+
75	95		// TestQuitMainLoopCallsPwMainLoopQuit verifies that QuitMainLoop delegates
76	96		// to the generated pw_main_loop_quit binding.
77	97		func TestQuitMainLoopCallsPwMainLoopQuit(t *testing.T) {

 	}
 	ops := &streamOpsImpl{
-		pinned:    make(map[unsafe.Pointer]*streamCallbackStorage),
+		pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
-		destroyed: make(map[unsafe.Pointer]bool),
 	}
 	fakePtr := unsafe.Pointer(uintptr(0x1234))
 		t.Fatalf("pw_stream_destroy received %v, want %v", receivedPtr, fakePtr)
 	}
-	// Verify internal bookkeeping.
+	// Verify internal bookkeeping: pinned entry should be removed.
-	if !ops.destroyed[fakePtr] {
-		t.Error("expected stream to be marked as destroyed")
-	}
 	if _, exists := ops.pinned[fakePtr]; exists {
 		t.Error("expected callback storage to be unpinned after destroy")
 	}
 	}
 	ops := &streamOpsImpl{
-		pinned:    make(map[unsafe.Pointer]*streamCallbackStorage),
+		pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
-		destroyed: make(map[unsafe.Pointer]bool),
 	}
 	fakePtr := unsafe.Pointer(uintptr(0xABCD))
 	}
 }
+// TestDestroyStreamBookkeepingDoesNotLeak verifies that internal tracking
+// does not retain entries after cleanup across many create/destroy cycles.
+func TestDestroyStreamBookkeepingDoesNotLeak(t *testing.T) {
+	origDestroy := pw_stream_destroy
+	t.Cleanup(func() { pw_stream_destroy = origDestroy })
+	pw_stream_destroy = func(_ unsafe.Pointer) {}
+	ops := &streamOpsImpl{
+		pinned: make(map[unsafe.Pointer]*streamCallbackStorage),
+	}
+	const cycles = 100
+	for i := uintptr(1); i <= cycles; i++ {
+		ptr := unsafe.Pointer(i)
+		ops.pinned[ptr] = &streamCallbackStorage{}
+		ops.DestroyStream(ptr)
+	}
+	// After destroying all streams, pinned map should be empty.
+	if len(ops.pinned) != 0 {
+		t.Fatalf("pinned map has %d entries after %d cycles; expected 0", len(ops.pinned), cycles)
+	}
+}
 // TestQuitMainLoopCallsPwMainLoopQuit verifies that QuitMainLoop delegates
 // to the generated pw_main_loop_quit binding.
 func TestQuitMainLoopCallsPwMainLoopQuit(t *testing.T) {

M internal/core/player.go +18 -2


@@ -213,9 +213,25 @@ func (p *player) deactivateStream() error {
213	213		return p.streamOps.SetStreamActive(p.streamPtr, false)
214	214		}
215	215
216		-	// teardown performs cleanup (placeholder)
	216	+	// teardown releases player-owned stream and main-loop resources through
	217	+	// StreamOps. Safe to call when fields are nil or on repeated invocation.
217	218		func (p *player) teardown() {
218		-	// Minimal placeholder - will be implemented in future tasks
	219	+	if p.streamOps == nil {
	220	+	return
	221	+	}
	222	+
	223	+	// Destroy stream first (it depends on the loop).
	224	+	if p.streamPtr != nil {
	225	+	p.streamOps.DestroyStream(p.streamPtr)
	226	+	p.streamPtr = nil
	227	+	}
	228	+
	229	+	// Quit then destroy the main loop.
	230	+	if p.loopPtr != nil {
	231	+	p.streamOps.QuitMainLoop(p.loopPtr)
	232	+	p.streamOps.DestroyMainLoop(p.loopPtr)
	233	+	p.loopPtr = nil
	234	+	}
219	235		}
220	236
221	237		// Player is the exported wrapper for the internal player


@@ -213,9 +213,25 @@ func (p *player) deactivateStream() error {
213		return p.streamOps.SetStreamActive(p.streamPtr, false)	213		return p.streamOps.SetStreamActive(p.streamPtr, false)
214		}	214		}
215			215
216	-	// teardown performs cleanup (placeholder)	216	+	// teardown releases player-owned stream and main-loop resources through
			217	+	// StreamOps. Safe to call when fields are nil or on repeated invocation.
217		func (p *player) teardown() {	218		func (p *player) teardown() {
218	-	// Minimal placeholder - will be implemented in future tasks	219	+	if p.streamOps == nil {
			220	+	return
			221	+	}
			222	+
			223	+	// Destroy stream first (it depends on the loop).
			224	+	if p.streamPtr != nil {
			225	+	p.streamOps.DestroyStream(p.streamPtr)
			226	+	p.streamPtr = nil
			227	+	}
			228	+
			229	+	// Quit then destroy the main loop.
			230	+	if p.loopPtr != nil {
			231	+	p.streamOps.QuitMainLoop(p.loopPtr)
			232	+	p.streamOps.DestroyMainLoop(p.loopPtr)
			233	+	p.loopPtr = nil
			234	+	}
219		}	235		}
220			236
221		// Player is the exported wrapper for the internal player	237		// Player is the exported wrapper for the internal player

M internal/core/player_test.go +123 -0


@@ -247,5 +247,128 @@ func TestPlayerStopWithoutStreamOpsIsNoop(t *testing.T) {
247	247		}
248	248		}
249	249
	250	+	// TestPlayerCloseTeardownCallsStreamOps verifies that Close() calls
	251	+	// DestroyStream, QuitMainLoop, and DestroyMainLoop through StreamOps
	252	+	// when the corresponding pointers are set.
	253	+	func TestPlayerCloseTeardownCallsStreamOps(t *testing.T) {
	254	+	mockOps := mocks.NewMockStreamOps(t)
	255	+	fakeStream := unsafe.Pointer(uintptr(0xAAAA))
	256	+	fakeLoop := unsafe.Pointer(uintptr(0xBBBB))
	257	+
	258	+	// Expect teardown calls in order: DestroyStream, QuitMainLoop, DestroyMainLoop.
	259	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
	260	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
	261	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
	262	+
	263	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
	264	+	p.streamOps = mockOps
	265	+	p.streamPtr = fakeStream
	266	+	p.loopPtr = fakeLoop
	267	+
	268	+	// Move to a state that allows Close.
	269	+	p.setState(PlayerStatePlaying)
	270	+
	271	+	if err := p.Close(); err != nil {
	272	+	t.Fatalf("Close failed: %v", err)
	273	+	}
	274	+	if p.State() != PlayerStateClosed {
	275	+	t.Fatalf("expected Closed state, got %v", p.State())
	276	+	}
	277	+
	278	+	// Pointers should be nil after teardown so repeated close is safe.
	279	+	if p.streamPtr != nil {
	280	+	t.Error("expected streamPtr to be nil after teardown")
	281	+	}
	282	+	if p.loopPtr != nil {
	283	+	t.Error("expected loopPtr to be nil after teardown")
	284	+	}
	285	+	}
	286	+
	287	+	// TestPlayerCloseWithNilPointersIsNoop verifies that teardown is safe
	288	+	// when no stream/loop pointers are set.
	289	+	func TestPlayerCloseWithNilPointersIsNoop(t *testing.T) {
	290	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
	291	+	p.setState(PlayerStatePlaying)
	292	+
	293	+	// No streamOps, no pointers — Close should succeed without panic.
	294	+	if err := p.Close(); err != nil {
	295	+	t.Fatalf("Close failed: %v", err)
	296	+	}
	297	+	if p.State() != PlayerStateClosed {
	298	+	t.Fatalf("expected Closed state, got %v", p.State())
	299	+	}
	300	+	}
	301	+
	302	+	// TestPlayerRepeatedCloseIsSafe verifies that calling Close() twice
	303	+	// with teardown resources does not call StreamOps methods a second time.
	304	+	func TestPlayerRepeatedCloseIsSafe(t *testing.T) {
	305	+	mockOps := mocks.NewMockStreamOps(t)
	306	+	fakeStream := unsafe.Pointer(uintptr(0xCCCC))
	307	+	fakeLoop := unsafe.Pointer(uintptr(0xDDDD))
	308	+
	309	+	// Only expect one set of teardown calls.
	310	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
	311	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
	312	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
	313	+
	314	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
	315	+	p.streamOps = mockOps
	316	+	p.streamPtr = fakeStream
	317	+	p.loopPtr = fakeLoop
	318	+	p.setState(PlayerStatePlaying)
	319	+
	320	+	if err := p.Close(); err != nil {
	321	+	t.Fatalf("first Close failed: %v", err)
	322	+	}
	323	+
	324	+	// Second close should succeed and not call StreamOps again.
	325	+	if err := p.Close(); err != nil {
	326	+	t.Fatalf("second Close failed: %v", err)
	327	+	}
	328	+	}
	329	+
	330	+	// TestPlayerTeardownWithOnlyStreamPtr verifies teardown works
	331	+	// when only streamPtr is set (no loopPtr).
	332	+	func TestPlayerTeardownWithOnlyStreamPtr(t *testing.T) {
	333	+	mockOps := mocks.NewMockStreamOps(t)
	334	+	fakeStream := unsafe.Pointer(uintptr(0xEEEE))
	335	+
	336	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
	337	+
	338	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
	339	+	p.streamOps = mockOps
	340	+	p.streamPtr = fakeStream
	341	+	p.setState(PlayerStatePlaying)
	342	+
	343	+	if err := p.Close(); err != nil {
	344	+	t.Fatalf("Close failed: %v", err)
	345	+	}
	346	+	if p.streamPtr != nil {
	347	+	t.Error("expected streamPtr to be nil after teardown")
	348	+	}
	349	+	}
	350	+
	351	+	// TestPlayerTeardownWithOnlyLoopPtr verifies teardown works
	352	+	// when only loopPtr is set (no streamPtr).
	353	+	func TestPlayerTeardownWithOnlyLoopPtr(t *testing.T) {
	354	+	mockOps := mocks.NewMockStreamOps(t)
	355	+	fakeLoop := unsafe.Pointer(uintptr(0xFFFF))
	356	+
	357	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
	358	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
	359	+
	360	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
	361	+	p.streamOps = mockOps
	362	+	p.loopPtr = fakeLoop
	363	+	p.setState(PlayerStatePlaying)
	364	+
	365	+	if err := p.Close(); err != nil {
	366	+	t.Fatalf("Close failed: %v", err)
	367	+	}
	368	+	if p.loopPtr != nil {
	369	+	t.Error("expected loopPtr to be nil after teardown")
	370	+	}
	371	+	}
	372	+
250	373		// Ensure mock.Mock is referenced to satisfy the import.
251	374		var _ mock.TestingT = (*testing.T)(nil)


@@ -247,5 +247,128 @@ func TestPlayerStopWithoutStreamOpsIsNoop(t *testing.T) {
247	}	247		}
248	}	248		}
249		249
		250	+	// TestPlayerCloseTeardownCallsStreamOps verifies that Close() calls
		251	+	// DestroyStream, QuitMainLoop, and DestroyMainLoop through StreamOps
		252	+	// when the corresponding pointers are set.
		253	+	func TestPlayerCloseTeardownCallsStreamOps(t *testing.T) {
		254	+	mockOps := mocks.NewMockStreamOps(t)
		255	+	fakeStream := unsafe.Pointer(uintptr(0xAAAA))
		256	+	fakeLoop := unsafe.Pointer(uintptr(0xBBBB))
		257	+
		258	+	// Expect teardown calls in order: DestroyStream, QuitMainLoop, DestroyMainLoop.
		259	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
		260	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
		261	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
		262	+
		263	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
		264	+	p.streamOps = mockOps
		265	+	p.streamPtr = fakeStream
		266	+	p.loopPtr = fakeLoop
		267	+
		268	+	// Move to a state that allows Close.
		269	+	p.setState(PlayerStatePlaying)
		270	+
		271	+	if err := p.Close(); err != nil {
		272	+	t.Fatalf("Close failed: %v", err)
		273	+	}
		274	+	if p.State() != PlayerStateClosed {
		275	+	t.Fatalf("expected Closed state, got %v", p.State())
		276	+	}
		277	+
		278	+	// Pointers should be nil after teardown so repeated close is safe.
		279	+	if p.streamPtr != nil {
		280	+	t.Error("expected streamPtr to be nil after teardown")
		281	+	}
		282	+	if p.loopPtr != nil {
		283	+	t.Error("expected loopPtr to be nil after teardown")
		284	+	}
		285	+	}
		286	+
		287	+	// TestPlayerCloseWithNilPointersIsNoop verifies that teardown is safe
		288	+	// when no stream/loop pointers are set.
		289	+	func TestPlayerCloseWithNilPointersIsNoop(t *testing.T) {
		290	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
		291	+	p.setState(PlayerStatePlaying)
		292	+
		293	+	// No streamOps, no pointers — Close should succeed without panic.
		294	+	if err := p.Close(); err != nil {
		295	+	t.Fatalf("Close failed: %v", err)
		296	+	}
		297	+	if p.State() != PlayerStateClosed {
		298	+	t.Fatalf("expected Closed state, got %v", p.State())
		299	+	}
		300	+	}
		301	+
		302	+	// TestPlayerRepeatedCloseIsSafe verifies that calling Close() twice
		303	+	// with teardown resources does not call StreamOps methods a second time.
		304	+	func TestPlayerRepeatedCloseIsSafe(t *testing.T) {
		305	+	mockOps := mocks.NewMockStreamOps(t)
		306	+	fakeStream := unsafe.Pointer(uintptr(0xCCCC))
		307	+	fakeLoop := unsafe.Pointer(uintptr(0xDDDD))
		308	+
		309	+	// Only expect one set of teardown calls.
		310	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
		311	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
		312	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
		313	+
		314	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
		315	+	p.streamOps = mockOps
		316	+	p.streamPtr = fakeStream
		317	+	p.loopPtr = fakeLoop
		318	+	p.setState(PlayerStatePlaying)
		319	+
		320	+	if err := p.Close(); err != nil {
		321	+	t.Fatalf("first Close failed: %v", err)
		322	+	}
		323	+
		324	+	// Second close should succeed and not call StreamOps again.
		325	+	if err := p.Close(); err != nil {
		326	+	t.Fatalf("second Close failed: %v", err)
		327	+	}
		328	+	}
		329	+
		330	+	// TestPlayerTeardownWithOnlyStreamPtr verifies teardown works
		331	+	// when only streamPtr is set (no loopPtr).
		332	+	func TestPlayerTeardownWithOnlyStreamPtr(t *testing.T) {
		333	+	mockOps := mocks.NewMockStreamOps(t)
		334	+	fakeStream := unsafe.Pointer(uintptr(0xEEEE))
		335	+
		336	+	mockOps.EXPECT().DestroyStream(fakeStream).Return()
		337	+
		338	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
		339	+	p.streamOps = mockOps
		340	+	p.streamPtr = fakeStream
		341	+	p.setState(PlayerStatePlaying)
		342	+
		343	+	if err := p.Close(); err != nil {
		344	+	t.Fatalf("Close failed: %v", err)
		345	+	}
		346	+	if p.streamPtr != nil {
		347	+	t.Error("expected streamPtr to be nil after teardown")
		348	+	}
		349	+	}
		350	+
		351	+	// TestPlayerTeardownWithOnlyLoopPtr verifies teardown works
		352	+	// when only loopPtr is set (no streamPtr).
		353	+	func TestPlayerTeardownWithOnlyLoopPtr(t *testing.T) {
		354	+	mockOps := mocks.NewMockStreamOps(t)
		355	+	fakeLoop := unsafe.Pointer(uintptr(0xFFFF))
		356	+
		357	+	mockOps.EXPECT().QuitMainLoop(fakeLoop).Return()
		358	+	mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return()
		359	+
		360	+	p := newPlayer(PlayerConfig{}, PlayerCallbacks{})
		361	+	p.streamOps = mockOps
		362	+	p.loopPtr = fakeLoop
		363	+	p.setState(PlayerStatePlaying)
		364	+
		365	+	if err := p.Close(); err != nil {
		366	+	t.Fatalf("Close failed: %v", err)
		367	+	}
		368	+	if p.loopPtr != nil {
		369	+	t.Error("expected loopPtr to be nil after teardown")
		370	+	}
		371	+	}
		372	+
250	// Ensure mock.Mock is referenced to satisfy the import.	373		// Ensure mock.Mock is referenced to satisfy the import.
251	var _ mock.TestingT = (*testing.T)(nil)	374		var _ mock.TestingT = (*testing.T)(nil)