internal: adds utility for refcounting and cleanup#9189
internal: adds utility for refcounting and cleanup#9189eshitachandwani wants to merge 5 commits into
Conversation
eshitachandwani
added
Type: Feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9189 +/- ##
==========================================
+ Coverage 83.09% 83.23% +0.13%
==========================================
Files 419 421 +2
Lines 33858 34055 +197
==========================================
+ Hits 28134 28345 +211
+ Misses 4291 4277 -14
Partials 1433 1433
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new generic RefCounted utility in internal/grpcsync/refcounted.go to manage resource lifecycles via atomic reference counting, accompanied by comprehensive unit tests. The review feedback focuses on enhancing safety and robustness by suggesting defensive programming checks: panicking if Increment is called on a dead resource, panicking if Decrement drops the reference count below zero, and validating that the onZero callback is not nil during initialization.
| // drops to zero. | ||
| func NewRefCounted[V any](val V, onZero func()) (*RefCounted[V], error) { | ||
| if onZero == nil { | ||
| return nil, fmt.Errorf("onZero callback cannot be nil") |
There was a problem hiding this comment.
Nit: prefix error with package name
| "sync/atomic" | ||
| ) | ||
|
|
||
| // RefCounted manages the lifecycle of a resource using atomic reference |
There was a problem hiding this comment.
Nit: Does the term "resource" apply here? This should be a generic utility that can be used to share any value among multiple users and ensure that the value is kept alive until there is at least one user.
There was a problem hiding this comment.
I'm not aware if 'resource' has another specific meaning that I am missing , but I felt it was helpful here to distinguish it from a regular 'value'. Since Go's GC handles standard values automatically, 'resource' helps signal that this item needs explicit lifecycle management and teardown (via the onZero callback). What do you think?
There was a problem hiding this comment.
Can this go something like: "RefCounted tracks how many consumers hold a value of type V and runs a cleanup when the last reference is released"
| // NewRefCounted creates a new RefCounted instance wrapping the given value. The | ||
| // provided onZero callback is executed exactly once when the reference count | ||
| // drops to zero. | ||
| func NewRefCounted[V any](val V, onZero func()) (*RefCounted[V], error) { |
There was a problem hiding this comment.
Nit: Should this docstring carry any recommendation about the type of V that makes sense here. For example, if someone stores something by value instead of by reference, it probably doesn't make much sense, right?
There was a problem hiding this comment.
Right! CHanged. Let me know if it is better ?
| // drops to zero. | ||
| func NewRefCounted[V any](val V, onZero func()) (*RefCounted[V], error) { | ||
| if onZero == nil { | ||
| return nil, fmt.Errorf("onZero callback cannot be nil") |
There was a problem hiding this comment.
Nit: this should also be specified in the docstring.
| // This method guarantees safe concurrent access by utilizing a CompareAndSwap | ||
| // loop. This prevents race conditions where a concurrent decrement could drop | ||
| // the count to zero between the read and the increment operation, which would | ||
| // otherwise inadvertently resurrect a closed resource. |
There was a problem hiding this comment.
This seems like a load of implementation detail. Should this be part of the docstring or can we move it inside the method and have something more palatable to users of this package?
| } | ||
|
|
||
| // Decrement decrements the reference count. If it drops to zero, the onZero | ||
| // callback is executed. |
There was a problem hiding this comment.
Nit: Mention that the callback is executed before this method returns.
| } | ||
|
|
||
| var wg sync.WaitGroup | ||
| wg.Add(numGoroutines) |
There was a problem hiding this comment.
Nit: Consider using the newly added wg.Go() instead.
| if err == nil { | ||
| t.Fatalf("NewRefCounted(_, nil) succeeded, want error") | ||
| } | ||
| expectedErr := "onZero callback cannot be nil" |
There was a problem hiding this comment.
Nit: s/expectedErr/wantErr
Also, make it a const.
| t.Fatalf("NewRefCounted(_, nil) succeeded, want error") | ||
| } | ||
| expectedErr := "onZero callback cannot be nil" | ||
| if err.Error() != expectedErr { |
There was a problem hiding this comment.
Combine this check with err == nil
| "sync/atomic" | ||
| ) | ||
|
|
||
| // RefCounted manages the lifecycle of a resource using atomic reference |
There was a problem hiding this comment.
Can this go something like: "RefCounted tracks how many consumers hold a value of type V and runs a cleanup when the last reference is released"
| } | ||
|
|
||
| // Value returns the encapsulated resource. | ||
| func (rc *RefCounted[V]) Value() V { |
There was a problem hiding this comment.
what happens if after onZero the value is a pointer that points to a cleaned up resource?
There was a problem hiding this comment.
It will return nil , but that is expected I think.
| "testing" | ||
| ) | ||
|
|
||
| // TestRefCounted_Basic tests the scenario where a RefCounted instance is |
There was a problem hiding this comment.
The test name is only TestRefCounted
4 participants
This PR adds the utility for refcounting objects and cleanups when the references go to zero.
For now it will be used for channel retention in A93 :Ext proc.
#ext-proc-a93
RELEASE NOTES: None