Skip to content

Commit fbe7f5a

Browse files
leggetterleggetter
committed
refactor(listen): fix remaining PR feedback issues
Address 5 additional issues from PR #186 review: 1. Add defensive nil check in FormatHealthMessage to prevent panic 2. Remove misleading TestCheckServerHealth_Timeout test that didn't actually test timeout behavior (TCP handshake succeeded immediately) 3. Fix goroutine leak by ensuring health monitor only starts once, not on every websocket reconnection 4. Use atomic.Swap instead of separate Load/Store to prevent race condition in state change detection 5. Reorganize into healthcheck subpackage and deduplicate logic Structural changes: - Created pkg/listen/healthcheck/ subpackage to avoid import cycles - Moved health check implementation to subpackage - Added re-export wrapper in pkg/listen/healthcheck.go for compatibility - Updated proxy to use healthcheck package function All tests passing. No API changes.
1 parent dfea4ee commit fbe7f5a

File tree

4 files changed

+129
-118
lines changed

4 files changed

+129
-118
lines changed

‎pkg/listen/healthcheck.go‎

Lines changed: 11 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1,76 +1,29 @@
11
package listen
22

33
import (
4-
"fmt"
5-
"net"
64
"net/url"
75
"time"
6+
7+
"github.com/hookdeck/hookdeck-cli/pkg/listen/healthcheck"
88
)
99

10-
// ServerHealthStatus represents the health status of the target server
11-
type ServerHealthStatus int
10+
// Re-export types and constants from healthcheck subpackage for backward compatibility
11+
type ServerHealthStatus = healthcheck.ServerHealthStatus
12+
type HealthCheckResult = healthcheck.HealthCheckResult
1213

1314
const (
14-
HealthHealthy ServerHealthStatus = iota // TCP connection successful
15-
HealthUnreachable // Connection refused or timeout
15+
HealthHealthy = healthcheck.HealthHealthy
16+
HealthUnreachable = healthcheck.HealthUnreachable
1617
)
1718

18-
// HealthCheckResult contains the result of a health check
19-
type HealthCheckResult struct {
20-
Status ServerHealthStatus
21-
Healthy bool
22-
Error error
23-
Timestamp time.Time
24-
Duration time.Duration
25-
}
26-
2719
// CheckServerHealth performs a TCP connection check to the target URL
20+
// This is a wrapper around the healthcheck package function for backward compatibility
2821
func CheckServerHealth(targetURL *url.URL, timeout time.Duration) HealthCheckResult {
29-
start := time.Now()
30-
31-
host := targetURL.Hostname()
32-
port := targetURL.Port()
33-
34-
// Default ports if not specified
35-
if port == "" {
36-
if targetURL.Scheme == "https" {
37-
port = "443"
38-
} else {
39-
port = "80"
40-
}
41-
}
42-
43-
address := net.JoinHostPort(host, port)
44-
45-
conn, err := net.DialTimeout("tcp", address, timeout)
46-
duration := time.Since(start)
47-
48-
result := HealthCheckResult{
49-
Timestamp: start,
50-
Duration: duration,
51-
}
52-
53-
if err != nil {
54-
result.Healthy = false
55-
result.Error = err
56-
result.Status = HealthUnreachable
57-
return result
58-
}
59-
60-
// Successfully connected - server is healthy
61-
conn.Close()
62-
result.Healthy = true
63-
result.Status = HealthHealthy
64-
return result
22+
return healthcheck.CheckServerHealth(targetURL, timeout)
6523
}
6624

6725
// FormatHealthMessage creates a user-friendly health status message
26+
// This is a wrapper around the healthcheck package function for backward compatibility
6827
func FormatHealthMessage(result HealthCheckResult, targetURL *url.URL) string {
69-
if result.Healthy {
70-
return fmt.Sprintf("✓ Local server is reachable at %s", targetURL.String())
71-
}
72-
73-
return fmt.Sprintf("⚠ Warning: Cannot connect to local server at %s\n %s\n The server may not be running. Events will fail until the server starts.",
74-
targetURL.String(),
75-
result.Error.Error())
28+
return healthcheck.FormatHealthMessage(result, targetURL)
7629
}

‎pkg/listen/healthcheck/healthcheck.go‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package healthcheck
2+
3+
import (
4+
"fmt"
5+
"net"
6+
"net/url"
7+
"time"
8+
)
9+
10+
// ServerHealthStatus represents the health status of the target server
11+
type ServerHealthStatus int
12+
13+
const (
14+
HealthHealthy ServerHealthStatus = iota // TCP connection successful
15+
HealthUnreachable // Connection refused or timeout
16+
)
17+
18+
// HealthCheckResult contains the result of a health check
19+
type HealthCheckResult struct {
20+
Status ServerHealthStatus
21+
Healthy bool
22+
Error error
23+
Timestamp time.Time
24+
Duration time.Duration
25+
}
26+
27+
// CheckServerHealth performs a TCP connection check to the target URL
28+
func CheckServerHealth(targetURL *url.URL, timeout time.Duration) HealthCheckResult {
29+
start := time.Now()
30+
31+
host := targetURL.Hostname()
32+
port := targetURL.Port()
33+
34+
// Default ports if not specified
35+
if port == "" {
36+
if targetURL.Scheme == "https" {
37+
port = "443"
38+
} else {
39+
port = "80"
40+
}
41+
}
42+
43+
address := net.JoinHostPort(host, port)
44+
45+
conn, err := net.DialTimeout("tcp", address, timeout)
46+
duration := time.Since(start)
47+
48+
result := HealthCheckResult{
49+
Timestamp: start,
50+
Duration: duration,
51+
}
52+
53+
if err != nil {
54+
result.Healthy = false
55+
result.Error = err
56+
result.Status = HealthUnreachable
57+
return result
58+
}
59+
60+
// Successfully connected - server is healthy
61+
conn.Close()
62+
result.Healthy = true
63+
result.Status = HealthHealthy
64+
return result
65+
}
66+
67+
// FormatHealthMessage creates a user-friendly health status message
68+
func FormatHealthMessage(result HealthCheckResult, targetURL *url.URL) string {
69+
if result.Healthy {
70+
return fmt.Sprintf("✓ Local server is reachable at %s", targetURL.String())
71+
}
72+
73+
errorMessage := "unknown error"
74+
if result.Error != nil {
75+
errorMessage = result.Error.Error()
76+
}
77+
return fmt.Sprintf("⚠ Warning: Cannot connect to local server at %s\n %s\n The server may not be running. Events will fail until the server starts.",
78+
targetURL.String(),
79+
errorMessage)
80+
}

pkg/listen/healthcheck_test.go renamed to pkg/listen/healthcheck/healthcheck_test.go

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
1-
package listen
1+
package healthcheck
22

33
import (
4-
"fmt"
54
"net"
65
"net/http"
76
"net/http/httptest"
@@ -112,31 +111,6 @@ func TestCheckServerHealth_DefaultPorts(t *testing.T) {
112111
}
113112
}
114113

115-
func TestCheckServerHealth_Timeout(t *testing.T) {
116-
// Create a listener that accepts but doesn't respond
117-
listener, err := net.Listen("tcp", "localhost:0")
118-
if err != nil {
119-
t.Fatalf("Failed to create listener: %v", err)
120-
}
121-
defer listener.Close()
122-
123-
// Get the actual port
124-
addr := listener.Addr().(*net.TCPAddr)
125-
targetURL, err := url.Parse(fmt.Sprintf("http://localhost:%d", addr.Port))
126-
if err != nil {
127-
t.Fatalf("Failed to parse URL: %v", err)
128-
}
129-
130-
// Use a very short timeout for fast test execution
131-
result := CheckServerHealth(targetURL, 10*time.Millisecond)
132-
133-
// The connection should succeed since we have a listener
134-
// This test mainly verifies that timeout is respected
135-
if result.Duration > 1*time.Second {
136-
t.Errorf("Health check took too long: %v", result.Duration)
137-
}
138-
}
139-
140114
func TestFormatHealthMessage_Healthy(t *testing.T) {
141115
targetURL, _ := url.Parse("http://localhost:3000")
142116
result := HealthCheckResult{
@@ -178,3 +152,21 @@ func TestFormatHealthMessage_Unhealthy(t *testing.T) {
178152
t.Errorf("Expected message to contain 'Warning'")
179153
}
180154
}
155+
156+
func TestFormatHealthMessage_NilError(t *testing.T) {
157+
targetURL, _ := url.Parse("http://localhost:3000")
158+
result := HealthCheckResult{
159+
Status: HealthUnreachable,
160+
Healthy: false,
161+
Error: nil, // Nil error should not cause panic
162+
}
163+
164+
msg := FormatHealthMessage(result, targetURL)
165+
166+
if len(msg) == 0 {
167+
t.Errorf("Expected non-empty message")
168+
}
169+
if !strings.Contains(msg, "unknown error") {
170+
t.Errorf("Expected message to contain 'unknown error' when error is nil")
171+
}
172+
}

‎pkg/listen/proxy/proxy.go‎

Lines changed: 19 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"fmt"
99
"io/ioutil"
1010
"math"
11-
"net"
1211
"net/http"
1312
"net/url"
1413
"os"
@@ -22,6 +21,7 @@ import (
2221
log "github.com/sirupsen/logrus"
2322

2423
"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
24+
"github.com/hookdeck/hookdeck-cli/pkg/listen/healthcheck"
2525
"github.com/hookdeck/hookdeck-cli/pkg/websocket"
2626
hookdecksdk "github.com/hookdeck/hookdeck-go-sdk"
2727
)
@@ -155,15 +155,20 @@ func (p *Proxy) Run(parentCtx context.Context) error {
155155
go func() {
156156
<-p.webSocketClient.Connected()
157157
p.renderer.OnConnected()
158-
hasConnectedOnce = true
159158

160-
// Perform initial health check and notify renderer immediately
161-
healthy, err := checkServerHealth(p.cfg.URL, 3*time.Second)
162-
p.serverHealthy.Store(healthy)
163-
p.renderer.OnServerHealthChanged(healthy, err)
159+
// Only start health monitoring on first successful connection
160+
// to prevent goroutine leaks on reconnects
161+
if !hasConnectedOnce {
162+
hasConnectedOnce = true
164163

165-
// Start health check monitor after initial check
166-
go p.startHealthCheckMonitor(signalCtx, p.cfg.URL)
164+
// Perform initial health check and notify renderer immediately
165+
healthy, err := checkServerHealth(p.cfg.URL, 3*time.Second)
166+
p.serverHealthy.Store(healthy)
167+
p.renderer.OnServerHealthChanged(healthy, err)
168+
169+
// Start health check monitor after initial check
170+
go p.startHealthCheckMonitor(signalCtx, p.cfg.URL)
171+
}
167172
}()
168173

169174
// Run the websocket in the background
@@ -449,29 +454,10 @@ func (p *Proxy) processEndpointResponse(eventID string, webhookEvent *websocket.
449454
}
450455
}
451456

452-
// checkServerHealth performs a simple TCP connection check to the target URL
453-
// This is a lightweight wrapper that extracts the host/port logic for reuse
457+
// checkServerHealth is a simple wrapper around the healthcheck package's CheckServerHealth
454458
func checkServerHealth(targetURL *url.URL, timeout time.Duration) (bool, error) {
455-
host := targetURL.Hostname()
456-
port := targetURL.Port()
457-
458-
// Default ports if not specified
459-
if port == "" {
460-
if targetURL.Scheme == "https" {
461-
port = "443"
462-
} else {
463-
port = "80"
464-
}
465-
}
466-
467-
address := net.JoinHostPort(host, port)
468-
conn, err := net.DialTimeout("tcp", address, timeout)
469-
if err != nil {
470-
return false, err
471-
}
472-
473-
conn.Close()
474-
return true, nil
459+
result := healthcheck.CheckServerHealth(targetURL, timeout)
460+
return result.Healthy, result.Error
475461
}
476462

477463
// startHealthCheckMonitor runs periodic health checks in the background
@@ -495,9 +481,9 @@ func (p *Proxy) startHealthCheckMonitor(ctx context.Context, targetURL *url.URL)
495481
// Perform health check
496482
healthy, err := checkServerHealth(targetURL, 3*time.Second)
497483

498-
// Only notify on state changes
499-
if healthy != p.serverHealthy.Load() {
500-
p.serverHealthy.Store(healthy)
484+
// Only notify on state changes, atomically
485+
prevHealthy := p.serverHealthy.Swap(healthy)
486+
if healthy != prevHealthy {
501487
p.renderer.OnServerHealthChanged(healthy, err)
502488

503489
// Adjust check interval based on health status

0 commit comments

Comments
 (0)