fix(metrics): add doc comments, fix labels mutation and nil returns, add idempotent Close, add nil guards, add tests

- Fix promMetrics.Gauge/Counter/Observer mutating caller's labels map by using maps.Copy instead of direct assignment - Fix Gauge/Counter/Observer returning nil for unknown metric names (now return noop implementations) - Fix metricService.Close() race with sync.Once for safe concurrent calls - Fix SetDSCP returning nil instead of errUnsupport on unsupported ops - Add nil guards to WrapConn, WrapPacketConn, WrapUDPConn, WrapListener - Add doc comments to all exported symbols - Add 45 unit tests (metrics, noop, prom, service, wrapper)
2026-05-24 22:50:41 +08:00
parent 64f9a048f8
commit bc3d12ec2c
12 changed files with 950 additions and 23 deletions
@@ -3,6 +3,7 @@ package service
 import (
 	"net"
 	"net/http"
+	"sync"

 	"github.com/go-gost/core/auth"
 	"github.com/go-gost/core/service"
@@ -10,6 +11,7 @@ import (
 )

 const (
+	// DefaultPath is the default HTTP path for the metrics endpoint.
 	DefaultPath = "/metrics"
 )

@@ -18,14 +20,17 @@ type options struct {
 	auther auth.Authenticator
 }

+// Option configures the metrics service.
 type Option func(*options)

+// PathOption sets the HTTP path for the metrics endpoint.
 func PathOption(path string) Option {
 	return func(o *options) {
 		o.path = path
 	}
 }

+// AutherOption sets the authenticator for the metrics endpoint.
 func AutherOption(auther auth.Authenticator) Option {
 	return func(o *options) {
 		o.auther = auther
@@ -33,11 +38,14 @@ func AutherOption(auther auth.Authenticator) Option {
 }

 type metricService struct {
-	s      *http.Server
-	ln     net.Listener
-	cclose chan struct{}
+	s        *http.Server
+	ln       net.Listener
+	cclose   chan struct{}
+	closeOnce sync.Once
+	closeErr  error
 }

+// NewService creates a metrics Service that exposes Prometheus metrics over HTTP.
 func NewService(network, addr string, opts ...Option) (service.Service, error) {
 	if network == "" {
 		network = "tcp"
@@ -75,18 +83,26 @@ func NewService(network, addr string, opts ...Option) (service.Service, error) {
 	}, nil
 }

+// Serve starts the metrics HTTP server and blocks until the listener is closed.
 func (s *metricService) Serve() error {
 	return s.s.Serve(s.ln)
 }

+// Addr returns the network address the metrics server is listening on.
 func (s *metricService) Addr() net.Addr {
 	return s.ln.Addr()
 }

+// Close stops the metrics HTTP server. It is safe to call multiple times.
 func (s *metricService) Close() error {
-	return s.s.Close()
+	s.closeOnce.Do(func() {
+		close(s.cclose)
+		s.closeErr = s.s.Close()
+	})
+	return s.closeErr
 }

+// IsClosed reports whether the metrics service has been closed.
 func (s *metricService) IsClosed() bool {
 	select {
 	case <-s.cclose:
@@ -0,0 +1,206 @@
+package service
+
+import (
+	"context"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/go-gost/core/auth"
+)
+
+// testAuther is a simple auth.Authenticator for testing.
+type testAuther struct {
+	allowUser, allowPass string
+}
+
+func (a *testAuther) Authenticate(ctx context.Context, user, password string, opts ...auth.Option) (string, bool) {
+	if user == a.allowUser && password == a.allowPass {
+		return user, true
+	}
+	return "", false
+}
+
+func TestNewServiceDefaultPath(t *testing.T) {
+	s, err := NewService("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+	defer s.Close()
+
+	if s.Addr() == nil {
+		t.Error("Addr should not be nil")
+	}
+}
+
+func TestNewServiceDefaultNetwork(t *testing.T) {
+	s, err := NewService("", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("NewService with empty network error: %v", err)
+	}
+	defer s.Close()
+
+	if s.Addr() == nil {
+		t.Error("Addr should not be nil")
+	}
+}
+
+func TestNewServiceCustomPath(t *testing.T) {
+	s, err := NewService("tcp", "127.0.0.1:0", PathOption("/custom"))
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+	defer s.Close()
+
+	// Serve in background for a short time.
+	go s.Serve()
+	time.Sleep(50 * time.Millisecond)
+
+	// Default path should 404.
+	resp, err := http.Get("http://" + s.Addr().String() + "/metrics")
+	if err != nil {
+		t.Fatalf("GET /metrics error: %v", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusNotFound {
+		t.Errorf("GET /metrics: expected 404, got %d", resp.StatusCode)
+	}
+
+	// Custom path should 200.
+	resp2, err := http.Get("http://" + s.Addr().String() + "/custom")
+	if err != nil {
+		t.Fatalf("GET /custom error: %v", err)
+	}
+	resp2.Body.Close()
+	if resp2.StatusCode != http.StatusOK {
+		t.Errorf("GET /custom: expected 200, got %d", resp2.StatusCode)
+	}
+}
+
+func TestNewServiceWithAuth(t *testing.T) {
+	auther := &testAuther{allowUser: "admin", allowPass: "secret"}
+	s, err := NewService("tcp", "127.0.0.1:0", AutherOption(auther))
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+	defer s.Close()
+
+	go s.Serve()
+	time.Sleep(50 * time.Millisecond)
+
+	addr := "http://" + s.Addr().String() + "/metrics"
+
+	// No credentials should be 401.
+	resp, err := http.Get(addr)
+	if err != nil {
+		t.Fatalf("GET error: %v", err)
+	}
+	resp.Body.Close()
+	if resp.StatusCode != http.StatusUnauthorized {
+		t.Errorf("expected 401 without credentials, got %d", resp.StatusCode)
+	}
+
+	// Wrong credentials should be 401.
+	req, _ := http.NewRequest("GET", addr, nil)
+	req.SetBasicAuth("admin", "wrong")
+	resp2, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("GET with wrong auth error: %v", err)
+	}
+	resp2.Body.Close()
+	if resp2.StatusCode != http.StatusUnauthorized {
+		t.Errorf("expected 401 with wrong credentials, got %d", resp2.StatusCode)
+	}
+
+	// Correct credentials should be 200.
+	req3, _ := http.NewRequest("GET", addr, nil)
+	req3.SetBasicAuth("admin", "secret")
+	resp3, err := http.DefaultClient.Do(req3)
+	if err != nil {
+		t.Fatalf("GET with correct auth error: %v", err)
+	}
+	resp3.Body.Close()
+	if resp3.StatusCode != http.StatusOK {
+		t.Errorf("expected 200 with correct credentials, got %d", resp3.StatusCode)
+	}
+}
+
+func TestServiceClose(t *testing.T) {
+	svc, err := NewService("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+	s := svc.(*metricService)
+
+	if s.IsClosed() {
+		t.Error("IsClosed should be false before Close")
+	}
+
+	// Serve in background.
+	go s.Serve()
+	time.Sleep(50 * time.Millisecond)
+
+	err = s.Close()
+	if err != nil {
+		t.Errorf("Close error: %v", err)
+	}
+
+	if !s.IsClosed() {
+		t.Error("IsClosed should be true after Close")
+	}
+
+	// Double close should be safe.
+	err = s.Close()
+	if err != nil {
+		t.Errorf("second Close error: %v", err)
+	}
+}
+
+func TestServiceServeReturnsOnClose(t *testing.T) {
+	s, err := NewService("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+
+	done := make(chan error, 1)
+	go func() {
+		done <- s.Serve()
+	}()
+
+	time.Sleep(50 * time.Millisecond)
+	s.Close()
+
+	select {
+	case err := <-done:
+		if err != nil && err != http.ErrServerClosed {
+			t.Errorf("Serve returned unexpected error: %v", err)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("Serve did not return within 2s after Close")
+	}
+}
+
+func TestNewServiceAddr(t *testing.T) {
+	s, err := NewService("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("NewService error: %v", err)
+	}
+	defer s.Close()
+
+	addr := s.Addr()
+	if addr == nil {
+		t.Fatal("Addr should not be nil")
+	}
+	if _, ok := addr.(*net.TCPAddr); !ok {
+		t.Errorf("expected *net.TCPAddr, got %T", addr)
+	}
+}
+
+func TestNewServiceListenError(t *testing.T) {
+	// Using an invalid address should cause a listen error.
+	_, err := NewService("invalid", "127.0.0.1:0")
+	if err == nil {
+		t.Error("expected error for invalid network, got nil")
+	}
+}