Repository: pion/interceptor
Pull request #264 (2024-07-23 03:57:40)

Unreference elements after priority queue Pop
Fixes https://github.com/pion/webrtc/issues/2778

cc: @at-wat 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/interceptor/pull/264?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 70.65%. Comparing base [(`0d85ad9`)](https://app.codecov.io/gh/pion/interceptor/commit/0d85ad9dbd2eda4a5394f2f9f6666d8859aa942f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`e36b6b3`)](https://app.codecov.io/gh/pion/interceptor/commit/e36b6b3206086e102e87edd6a90b10d4bd152760?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
- Coverage   72.39%   70.65%   -1.75%     
==========================================
  Files          79       79              
  Lines        3648     3653       +5     
==========================================
- Hits         2641     2581      -60     
- Misses        870      927      +57     
- Partials      137      145       +8     
```

| [Flag](https://app.codecov.io/gh/pion/interceptor/pull/264/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/interceptor/pull/264/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/interceptor/pull/264/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `70.65% <100.00%> (?)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/interceptor/pull/264?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/pkg/jitterbuffer/priority_queue.go b/pkg/jitterbuffer/priority_queue.go
index f6d7d93..11a8679 100644
--- a/pkg/jitterbuffer/priority_queue.go
+++ b/pkg/jitterbuffer/priority_queue.go
@@ -114,6 +114,7 @@ func (q *PriorityQueue) Pop() (*rtp.Packet, error) {
 		return nil, ErrInvalidOperation
 	}
 	val := q.next.val
+	q.next.val = nil
 	q.length--
 	q.next = q.next.next
 	return val, nil
@@ -126,6 +127,7 @@ func (q *PriorityQueue) PopAt(sqNum uint16) (*rtp.Packet, error) {
 	}
 	if q.next.priority == sqNum {
 		val := q.next.val
+		q.next.val = nil
 		q.next = q.next.next
 		q.length--
 		return val, nil
@@ -135,6 +137,7 @@ func (q *PriorityQueue) PopAt(sqNum uint16) (*rtp.Packet, error) {
 	for pos != nil {
 		if pos.priority == sqNum {
 			val := pos.val
+			pos.val = nil
 			prev.next = pos.next
 			if prev.next != nil {
 				prev.next.prev = prev
@@ -156,6 +159,7 @@ func (q *PriorityQueue) PopAtTimestamp(timestamp uint32) (*rtp.Packet, error) {
 	}
 	if q.next.val.Timestamp == timestamp {
 		val := q.next.val
+		q.next.val = nil
 		q.next = q.next.next
 		q.length--
 		return val, nil
@@ -165,6 +169,7 @@ func (q *PriorityQueue) PopAtTimestamp(timestamp uint32) (*rtp.Packet, error) {
 	for pos != nil {
 		if pos.val.Timestamp == timestamp {
 			val := pos.val
+			pos.val = nil
 			prev.next = pos.next
 			if prev.next != nil {
 				prev.next.prev = prev


Test patch:
diff --git a/pkg/jitterbuffer/priority_queue_test.go b/pkg/jitterbuffer/priority_queue_test.go
index 7fb2a7a..8b8d23e 100644
--- a/pkg/jitterbuffer/priority_queue_test.go
+++ b/pkg/jitterbuffer/priority_queue_test.go
@@ -4,7 +4,10 @@
 package jitterbuffer
 
 import (
+	"runtime"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/pion/rtp"
 	"github.com/stretchr/testify/assert"
@@ -136,3 +139,46 @@ func TestPriorityQueue_Clean(t *testing.T) {
 	assert.EqualValues(t, 1, packets.Length())
 	packets.Clear()
 }
+
+func TestPriorityQueue_Unreference(t *testing.T) {
+	packets := NewQueue()
+
+	var refs int64
+	finalizer := func(*rtp.Packet) {
+		atomic.AddInt64(&refs, -1)
+	}
+
+	numPkts := 100
+	for i := 0; i < numPkts; i++ {
+		atomic.AddInt64(&refs, 1)
+		seq := uint16(i)
+		p := rtp.Packet{
+			Header: rtp.Header{
+				SequenceNumber: seq,
+				Timestamp:      uint32(i + 42),
+			},
+			Payload: []byte{byte(i)},
+		}
+		runtime.SetFinalizer(&p, finalizer)
+		packets.Push(&p, seq)
+	}
+	for i := 0; i < numPkts-1; i++ {
+		switch i % 3 {
+		case 0:
+			packets.Pop() //nolint
+		case 1:
+			packets.PopAt(uint16(i)) //nolint
+		case 2:
+			packets.PopAtTimestamp(uint32(i + 42)) //nolint
+		}
+	}
+
+	runtime.GC()
+	time.Sleep(10 * time.Millisecond)
+
+	remainedRefs := atomic.LoadInt64(&refs)
+	runtime.KeepAlive(packets)
+
+	// only the last packet should be still referenced
+	assert.Equal(t, int64(1), remainedRefs)
+}