From 4b1add4a9ca3fba8005dc165a3506b7e8d240a8a Mon Sep 17 00:00:00 2001 From: Cytown Date: Sat, 28 Mar 2026 10:47:54 +0800 Subject: [PATCH] fix for review --- pkg/channels/dynamic_mux_test.go | 162 +++++++++++++++++++++++++++++++ pkg/channels/manager.go | 9 ++ 2 files changed, 171 insertions(+) create mode 100644 pkg/channels/dynamic_mux_test.go diff --git a/pkg/channels/dynamic_mux_test.go b/pkg/channels/dynamic_mux_test.go new file mode 100644 index 000000000..d895c69c9 --- /dev/null +++ b/pkg/channels/dynamic_mux_test.go @@ -0,0 +1,162 @@ +package channels + +import ( + "net/http" + "net/http/httptest" + "sync" + "testing" +) + +func TestDynamicServeMuxExactMatch(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/health", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rec.Code) + } +} + +func TestDynamicServeMuxSubtreePrefixMatch(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/api/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }) + + for _, path := range []string{"/api/", "/api/v1", "/api/v1/resource"} { + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if rec.Code != http.StatusCreated { + t.Fatalf("path %q: expected 201, got %d", path, rec.Code) + } + } +} + +func TestDynamicServeMuxExactOverPrefix(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/api", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + dm.HandleFunc("/api/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }) + + // Exact match wins + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("exact match: expected 200, got %d", rec.Code) + } + + // Prefix match for sub-paths + rec = httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/v1", nil)) + if rec.Code != http.StatusCreated { + t.Fatalf("prefix match: expected 201, got %d", rec.Code) + } +} + +func TestDynamicServeMuxLongestPrefixWins(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/a/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + dm.HandleFunc("/a/b/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + }) + + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/a/b/c", nil)) + if rec.Code != http.StatusAccepted { + t.Fatalf("longest prefix: expected 202, got %d", rec.Code) + } +} + +func TestDynamicServeMuxNotFound(t *testing.T) { + dm := newDynamicServeMux() + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/nonexistent", nil)) + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d", rec.Code) + } +} + +func TestDynamicServeMuxUnhandle(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Verify it works before removal + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/test", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("before unhandle: expected 200, got %d", rec.Code) + } + + // Remove and verify 404 + dm.Unhandle("/test") + rec = httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/test", nil)) + if rec.Code != http.StatusNotFound { + t.Fatalf("after unhandle: expected 404, got %d", rec.Code) + } +} + +func TestDynamicServeMuxConcurrent(t *testing.T) { + dm := newDynamicServeMux() + dm.HandleFunc("/static", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + var wg sync.WaitGroup + const goroutines = 50 + + // Concurrent Handle/Unhandle + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + pattern := "/concurrent" + if i%2 == 0 { + dm.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + }) + } else { + dm.Unhandle(pattern) + } + }(i) + } + + // Concurrent ServeHTTP + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/static", nil)) + // Should not panic; result is either 200 or 404 + _ = rec.Code + }() + } + + wg.Wait() +} + +func TestDynamicServeMuxHandleUsesHandler(t *testing.T) { + dm := newDynamicServeMux() + + var called bool + dm.Handle("/handler", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + })) + + rec := httptest.NewRecorder() + dm.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/handler", nil)) + if !called { + t.Fatal("handler was not called") + } +} diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index 1df6da59b..a011dd8b1 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -1017,6 +1017,10 @@ func (m *Manager) Reload(ctx context.Context, cfg *config.Config) error { m.mu.Lock() defer m.mu.Unlock() + // Save old config so we can revert on error. + oldConfig := m.config + + // Update config early: initChannel uses m.config via factory(m.config, m.bus). m.config = cfg list := toChannelHashes(cfg) @@ -1044,11 +1048,15 @@ func (m *Manager) Reload(ctx context.Context, cfg *config.Config) error { cc, err := toChannelConfig(cfg, added) if err != nil { logger.ErrorC("channels", fmt.Sprintf("toChannelConfig error: %v", err)) + m.config = oldConfig + cancel() return err } err = m.initChannels(cc) if err != nil { logger.ErrorC("channels", fmt.Sprintf("initChannels error: %v", err)) + m.config = oldConfig + cancel() return err } for _, name := range added { @@ -1073,6 +1081,7 @@ func (m *Manager) Reload(ctx context.Context, cfg *config.Config) error { }) } + // Commit hashes only on full success. m.channelHashes = list go func() { for _, f := range deferFuncs {