Copilot commented on code in PR #876:
URL: https://github.com/apache/dubbo-go-pixiu/pull/876#discussion_r2768733927
##########
pkg/filter/sentinel/circuitbreaker/circuit_breaker_test.go:
##########
@@ -74,3 +77,204 @@ func mockConfig() *Config {
}
return &c
}
+
+// mockConfigWithResource creates a test config with a custom resource name
+func mockConfigWithResource(resourceName string) *Config {
+ c := Config{
+ Resources: []*pkgs.Resource{
+ {
+ Name: resourceName,
+ Items: []*pkgs.Item{
+ {MatchStrategy: pkgs.EXACT, Pattern:
"/api/v1/" + resourceName + "/user"},
+ {MatchStrategy: pkgs.REGEX, Pattern:
"/api/v1/" + resourceName + "/user/*"},
+ },
+ },
+ },
+ Rules: []*circuitbreaker.Rule{{
+ Resource: resourceName,
+ Strategy: circuitbreaker.ErrorCount,
+ RetryTimeoutMs: 3000,
+ MinRequestAmount: 10,
+ StatIntervalMs: 1000,
+ Threshold: 1.0,
+ }},
+ }
+ return &c
+}
+
+// TestCircuitBreakerFeedbackLoop tests the complete feedback loop for circuit
breaker
+// This test verifies the fix for issue #869
+func TestCircuitBreakerFeedbackLoop(t *testing.T) {
+ // Setup
+ factory := FilterFactory{cfg: &Config{}}
+ mockYaml, err := yaml.MarshalYML(mockConfig())
+ require.NoError(t, err)
+ require.NoError(t, yaml.UnmarshalYML(mockYaml, factory.Config()))
+ require.NoError(t, factory.Apply())
+
+ f := &Filter{cfg: factory.cfg, matcher: factory.matcher}
+
+ t.Run("Decode stores entry in context", func(t *testing.T) {
+ request, _ := stdHttp.NewRequest(stdHttp.MethodGet,
"https://www.dubbogopixiu.com/api/v1/test-dubbo/user/1111";, nil)
+ ctx := mock.GetMockHTTPContext(request)
+
+ // Execute Decode
+ status := f.Decode(ctx)
+ assert.Equal(t, filter.Continue, status)
+
+ // Verify entry is stored in context
+ entryVal, exists := ctx.Params[ContextKeySentinelEntry]
+ assert.True(t, exists, "Sentinel entry should be stored in
context")
+ assert.NotNil(t, entryVal, "Sentinel entry should not be nil")
+
+ _, ok := entryVal.(*base.SentinelEntry)
+ assert.True(t, ok, "Context value should be a SentinelEntry")
Review Comment:
This test creates a Sentinel entry in Decode but never calls Encode, which
means entry.Exit() is never called. This creates a resource leak in the test.
The test should call both Decode and Encode to properly clean up the Sentinel
entry, similar to how the other test cases in this file are structured.
```suggestion
assert.True(t, ok, "Context value should be a SentinelEntry")
// Call Encode to ensure the Sentinel entry is properly exited
and cleaned up
ctx.StatusCode(200)
encodeStatus := f.Encode(ctx)
assert.Equal(t, filter.Continue, encodeStatus)
```
##########
pkg/filter/sentinel/circuitbreaker/circuit_breaker.go:
##########
@@ -101,7 +107,40 @@ func (f *Filter) Decode(ctx *http.HttpContext)
filter.FilterStatus {
ctx.SendLocalReply(errResp.Status, errResp.ToJSON())
return filter.Stop
}
+
+ // Store entry in context for later use in Encode phase
+ if ctx.Params == nil {
+ ctx.Params = make(map[string]any)
+ }
+ ctx.Params[ContextKeySentinelEntry] = entry
+
+ return filter.Continue
Review Comment:
If a panic occurs after Decode stores the Sentinel entry but before Encode
is called (e.g., in another filter or in buildTargetResponse), entry.Exit()
will never be called, causing a resource leak. The defer/recover in manager.go
catches panics but doesn't ensure OnEncode is called. Consider adding a defer
in Decode that checks if the entry was already cleaned up, or wrapping the
Entry/Exit lifecycle differently to ensure cleanup happens even in panic
scenarios.
##########
pkg/filter/sentinel/circuitbreaker/circuit_breaker.go:
##########
@@ -77,9 +81,11 @@ func (p *Plugin) CreateFilterFactory()
(filter.HttpFilterFactory, error) {
return &FilterFactory{cfg: &Config{}}, nil
}
-// Deep copy config to avoid pointer sharing (factory.cfg may change at
runtime)
+// deep copy config to avoid pointer sharing (factory.cfg may change at
runtime)
Review Comment:
The comment should start with a capital letter "Deep" to be consistent with
the codebase convention. All other similar comments in the codebase use "Deep
copy" (e.g., pkg/filter/sentinel/ratelimit/rate_limit.go:71,
pkg/filter/authority/authority.go:64, pkg/filter/cors/cors.go:76).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
