Alanxtl commented on code in PR #3695:
URL: https://github.com/apache/dubbo-go/pull/3695#discussion_r3840272730
##########
filter/graceful_shutdown/provider_filter.go:
##########
@@ -93,7 +93,7 @@ func (f *providerGracefulShutdownFilter) OnResponse(ctx
context.Context, res res
}
if shouldDecrementProviderActive(res) {
- f.shutdownConfig.ProviderActiveCount.Dec()
+ f.shutdownConfig.ProviderActiveCount.Add(-1)
Review Comment:
ditto
##########
filter/graceful_shutdown/consumer_filter.go:
##########
@@ -98,7 +98,7 @@ func (f *consumerGracefulShutdownFilter) Invoke(ctx
context.Context, invoker bas
// OnResponse reduces the number of active processes then return the process
result
func (f *consumerGracefulShutdownFilter) OnResponse(ctx context.Context,
result result.Result, invoker base.Invoker, invocation base.Invocation)
result.Result {
if f.shutdownConfig != nil && shouldDecrementConsumerActive(result) {
- f.shutdownConfig.ConsumerActiveCount.Dec()
+ f.shutdownConfig.ConsumerActiveCount.Add(-1)
Review Comment:
这两个为什么修改
##########
filter/generic/generalizer/bean.go:
##########
@@ -92,8 +92,8 @@ func (g *BeanGeneralizer) Realize(obj any, typ reflect.Type)
(any, error) {
func (g *BeanGeneralizer) GetType(obj any) (typ string, err error) {
typ, err = hessian2.GetJavaName(obj)
- // no error or error is not NilError
- if err == nil || err != hessian2.NilError {
+ // no error or error is not ErrNilError
+ if err == nil || err != hessian2.ErrNilError {
Review Comment:
`ErrNilError`是啥
`protocol\dubbo\hessian2\java_class.go`里面定义的不是
```
NilError = perrors.Errorf("object should not be nil")
```
##########
protocol/dubbo/hessian2/java_class.go:
##########
@@ -26,20 +26,18 @@ import (
import (
hessian "github.com/apache/dubbo-go-hessian2"
-
- perrors "github.com/pkg/errors"
)
var (
- NilError = perrors.Errorf("object should not be nil")
- UnexpectedTypeError = perrors.Errorf("object should be a POJO")
- notBasicClassError = perrors.Errorf("object isn't a basic class")
+ ErrNilError = fmt.Errorf("object should not be nil")
+ ErrUnexpectedTypeError = fmt.Errorf("object should be a POJO")
+ errNotBasicClassError = fmt.Errorf("object isn't a basic class")
Review Comment:
不要改名字
##########
proxy/proxy_test.go:
##########
@@ -159,3 +160,33 @@ func (bi *TestProxyInvoker) Invoke(_ context.Context, inv
base.Invocation) resul
Rest: inv.Arguments(),
}
}
+
+// TestResolveRootCause is a regression test for the stdlib replacement of
+// perrors.Cause in proxy.go: resolveRootCause must walk both the stdlib
+// Unwrap() and pkg/errors Cause() chains down to the root error.
+func TestResolveRootCause(t *testing.T) {
+ root := errors.New("root")
+
+ t.Run("plain error returns itself", func(t *testing.T) {
+ require.Equal(t, root, resolveRootCause(root))
+ })
+
+ t.Run("stdlib Unwrap chain", func(t *testing.T) {
+ wrapped := fmt.Errorf("w1: %w", fmt.Errorf("w2: %w", root))
+ require.Equal(t, root, resolveRootCause(wrapped))
+ })
+
+ t.Run("pkg/errors Cause chain", func(t *testing.T) {
+ wrapped := perrors.Wrapf(perrors.Wrap(root, "a"), "b")
+ require.Equal(t, root, resolveRootCause(wrapped))
+ })
+
+ t.Run("mixed Unwrap and Cause chain", func(t *testing.T) {
+ wrapped := fmt.Errorf("w1: %w", perrors.Wrap(root, "a"))
+ require.Equal(t, root, resolveRootCause(wrapped))
+ })
Review Comment:
既然我们把perrors删了 为什么还要兼容perrors
##########
registry/service_instance.go:
##########
@@ -240,7 +239,7 @@ func (d *DefaultServiceInstance) GetMetadata()
map[string]string {
// logic to modify instance. Be careful of priority. Usually you should use
number
// between [100, 9000] other number will be thought as system reserve number
type ServiceInstanceCustomizer interface {
- gxsort.Prioritizer
+ GetPriority() int
Review Comment:
还是抽出一个Prioritizer的interface吧
##########
proxy/proxy.go:
##########
@@ -59,6 +57,24 @@ type (
var typError = reflect.Zero(reflect.TypeFor[error]()).Type()
+// resolveRootCause returns the root cause of err, walking both the stdlib
+// Unwrap() and pkg/errors Cause() chains. It is equivalent to perrors.Cause.
+func resolveRootCause(err error) error {
+ cause := err
+ for cause != nil {
+ if u, ok := cause.(interface{ Unwrap() error }); ok {
+ cause = u.Unwrap()
+ continue
+ }
+ if c, ok := cause.(interface{ Cause() error }); ok {
+ cause = c.Cause()
+ continue
+ }
+ break
+ }
+ return cause
+}
Review Comment:
ditto
##########
filter/adaptivesvc/limiter/hill_climbing.go:
##########
@@ -50,6 +47,72 @@ var (
stablePeriod = 32000 * time.Millisecond
)
+// The standard library does not provide atomic types for float64,
+// time.Duration and time.Time. The following minimal wrappers keep the
+// HillClimbing limiter on top of sync/atomic primitives only.
+type atomicFloat64 struct {
+ bits atomic.Uint64
+}
+
+func newAtomicFloat64(v float64) *atomicFloat64 {
+ f := &atomicFloat64{}
+ f.bits.Store(math.Float64bits(v))
+ return f
+}
+
+func (f *atomicFloat64) Load() float64 {
+ return math.Float64frombits(f.bits.Load())
+}
+
+func (f *atomicFloat64) Store(v float64) {
+ f.bits.Store(math.Float64bits(v))
+}
+
+type atomicDuration struct {
+ nanos atomic.Int64
+}
Review Comment:
atomicFloat64 和atomicDuration 就保留uber atomic吧
##########
filter/adaptivesvc/limiter/hill_climbing.go:
##########
@@ -50,6 +47,72 @@ var (
stablePeriod = 32000 * time.Millisecond
)
+// The standard library does not provide atomic types for float64,
+// time.Duration and time.Time. The following minimal wrappers keep the
+// HillClimbing limiter on top of sync/atomic primitives only.
+type atomicFloat64 struct {
+ bits atomic.Uint64
+}
+
+func newAtomicFloat64(v float64) *atomicFloat64 {
+ f := &atomicFloat64{}
+ f.bits.Store(math.Float64bits(v))
+ return f
+}
+
+func (f *atomicFloat64) Load() float64 {
+ return math.Float64frombits(f.bits.Load())
+}
+
+func (f *atomicFloat64) Store(v float64) {
+ f.bits.Store(math.Float64bits(v))
+}
+
+type atomicDuration struct {
+ nanos atomic.Int64
+}
+
+func newAtomicDuration(d time.Duration) *atomicDuration {
+ a := &atomicDuration{}
+ a.nanos.Store(int64(d))
+ return a
+}
+
+func (d *atomicDuration) Load() time.Duration {
+ return time.Duration(d.nanos.Load())
+}
+
+func (d *atomicDuration) Store(v time.Duration) {
+ d.nanos.Store(int64(v))
+}
+
+type atomicTime struct {
+ t atomic.Pointer[time.Time]
+}
Review Comment:
这个没有必要包一层了吧
##########
common/host_util.go:
##########
@@ -44,10 +42,53 @@ func GetLocalIp() string {
if len(localIp) != 0 {
return localIp
}
- localIp, _ = gxnet.GetLocalIP()
+ localIp, _ = getLocalIP()
return localIp
}
+func getLocalIP() (string, error) {
Review Comment:
比旧版清晰了，但还不能算“漂亮且稳健”。有两个关键问题。
`getLocalIP`：
- 只处理了 `*net.IPNet`，还应支持 `*net.IPAddr`。
- `localIP` 会不断被覆盖，最终可能返回任意一个接口地址，选择策略不稳定。
- 应排除 `IsUnspecified()` 和 `IsLinkLocalUnicast()`。
- 仅通过接口名包含 `"docker"` 过滤比较脆弱，其他虚拟网卡仍可能被选中。
- `iface.Addrs()` 出错时直接返回，可能因为一个异常接口导致整体失败。
可以抽出地址解析：
```go
func ipFromAddr(addr net.Addr) net.IP {
switch v := addr.(type) {
case *net.IPNet:
return v.IP
case *net.IPAddr:
return v.IP
default:
return nil
}
}
```
`ListenOnTCPRandomPort` 有一个更明确的 bug：
```go
net.ParseIP(ip)
```
解析失败会返回 `nil`，之后可能退化成监听所有 IPv4 地址，错误被静默隐藏。建议校验：
```go
func ListenOnTCPRandomPort(ip string) (*net.TCPListener, error) {
addr := &net.TCPAddr{
IP: net.IPv4zero,
Port: 0,
}
if ip != "" {
parsed := net.ParseIP(ip)
if parsed == nil || parsed.To4() == nil {
return nil, fmt.Errorf("invalid IPv4 address %q", ip)
}
addr.IP = parsed.To4()
}
return net.ListenTCP("tcp4", addr)
}
```
##########
global/shutdown_config.go:
##########
@@ -92,6 +91,21 @@ func DefaultShutdownConfig() *ShutdownConfig {
return cfg
}
+// LoadLastReceivedRequestTime returns the timestamp of the last received
request.
+// A nil pointer means no request has ever been received, which is represented
as
+// the zero time.
+func (c *ShutdownConfig) LoadLastReceivedRequestTime() time.Time {
+ if last := c.ProviderLastReceivedRequestTime.Load(); last != nil {
+ return *last
+ }
+ return time.Time{}
+}
+
+// StoreLastReceivedRequestTime records the timestamp of the last received
request.
+func (c *ShutdownConfig) StoreLastReceivedRequestTime(t time.Time) {
+ c.ProviderLastReceivedRequestTime.Store(&t)
+}
Review Comment:
这个有必要包一层吗
##########
loader.go:
##########
@@ -67,6 +68,23 @@ var watcher = &fileWatcher{
stopCh: make(chan struct{}),
}
+// goSafely runs fn in a new goroutine and recovers from any panic it raises,
+// preserving the recover and WaitGroup semantics of the former
+// gost/runtime.GoSafely helper so that a panicking watcher cannot crash the
+// process.
+func goSafely(wg *sync.WaitGroup, fn func()) {
+ wg.Add(1)
+ go func() {
+ defer func() {
+ if r := recover(); r != nil {
+ fmt.Fprintf(os.Stderr, "%s goroutine panic:
%v\n%s\n", time.Now(), r, debug.Stack())
+ }
+ wg.Done()
+ }()
+ fn()
+ }()
+}
Review Comment:
```suggestion
// goSafely runs handler in a new goroutine, recovers panics raised while
// handler executes on that goroutine, and tracks completion in wg.
// wg must not be nil.
// See github.com/dubbogo/gost/runtime/goroutine.go.
func goSafely(wg *sync.WaitGroup, handler func()) {
wg.Go(func() {
defer func() {
if r := recover(); r != nil {
fmt.Fprintf(os.Stderr,
"%s goroutine panic: %v\n%s\n",
time.Now(), r, debug.Stack())
}
}()
handler()
})
}
```
##########
loader_test.go:
##########
@@ -108,3 +110,33 @@ func TestHotUpdateConfig_AllowsWithCustomPrefix(t
*testing.T) {
t.Fatalf("hotUpdateConfig unexpected error with allowed prefix:
%v", err)
}
}
+
+// TestGoSafely_RunsAndRecoversPanic is a regression test for the inline
+// replacement of gost/runtime.GoSafely in loader.go: fn must run, and a panic
+// raised inside fn must be recovered so that wg.Done is still reached and the
+// process is not crashed.
+func TestGoSafely_RunsAndRecoversPanic(t *testing.T) {
+ ran := make(chan struct{})
+ var wg sync.WaitGroup
+ goSafely(&wg, func() { close(ran) })
+ select {
+ case <-ran:
+ case <-time.After(time.Second):
+ t.Fatal("goSafely did not run the function")
+ }
+ wg.Wait()
+
+ started := make(chan struct{})
+ var wg2 sync.WaitGroup
+ goSafely(&wg2, func() {
+ close(started)
+ panic("boom")
+ })
+ select {
+ case <-started:
+ case <-time.After(time.Second):
+ t.Fatal("goSafely did not start the panicking function")
+ }
+ // Must not hang: recover() ensures wg.Done() is invoked even on panic.
+ wg2.Wait()
+}
Review Comment:
再把老的TestGoSafe测试也移过来
--
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]
