Skip to content

remove parent context from the httpserver Runnable #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions examples/http/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,27 +81,22 @@ func buildRoutes(logHandler slog.Handler) ([]httpserver.Route, error) {
}

// RunServer initializes and runs the HTTP server with supervisor
// Returns the supervisor and a cleanup function
func RunServer(
ctx context.Context,
logHandler slog.Handler,
routes []httpserver.Route,
) (*supervisor.PIDZero, func(), error) {
) (*supervisor.PIDZero, error) {
// Create a config callback function that will be used by the runner
configCallback := func() (*httpserver.Config, error) {
return httpserver.NewConfig(ListenOn, routes, httpserver.WithDrainTimeout(DrainTimeout))
}

// Create the HTTP server runner with a custom context
customCtx, customCancel := context.WithCancel(ctx)

// Create the HTTP server runner
runner, err := httpserver.NewRunner(
httpserver.WithContext(customCtx),
httpserver.WithConfigCallback(configCallback),
httpserver.WithLogHandler(logHandler))
if err != nil {
customCancel()
return nil, nil, fmt.Errorf("failed to create HTTP server runner: %w", err)
return nil, fmt.Errorf("failed to create HTTP server runner: %w", err)
}

// Create a PIDZero supervisor and add the runner
Expand All @@ -110,11 +105,10 @@ func RunServer(
supervisor.WithLogHandler(logHandler),
supervisor.WithRunnables(runner))
if err != nil {
customCancel()
return nil, nil, fmt.Errorf("failed to create supervisor: %w", err)
return nil, fmt.Errorf("failed to create supervisor: %w", err)
}

return sv, customCancel, nil
return sv, nil
}

func main() {
Expand All @@ -135,12 +129,11 @@ func main() {
os.Exit(1)
}

sv, cancel, err := RunServer(ctx, handler, routes)
sv, err := RunServer(ctx, handler, routes)
if err != nil {
slog.Error("Failed to setup server", "error", err)
os.Exit(1)
}
defer cancel()

// Start the supervisor - this will block until shutdown
slog.Info("Starting supervisor with HTTP server on " + ListenOn)
Expand Down
8 changes: 3 additions & 5 deletions examples/http/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@ func TestRunServer(t *testing.T) {
require.NoError(t, err, "Failed to build routes")
require.NotEmpty(t, routes, "Routes should not be empty")

sv, cleanup, err := RunServer(ctx, logHandler, routes)
sv, err := RunServer(ctx, logHandler, routes)
require.NoError(t, err, "RunServer should not return an error")
require.NotNil(t, sv, "Supervisor should not be nil")
require.NotNil(t, cleanup, "Cleanup function should not be nil")

// Start the server in a goroutine to avoid blocking the test
errCh := make(chan error, 1)
Expand All @@ -54,8 +53,8 @@ func TestRunServer(t *testing.T) {
assert.Equal(t, http.StatusOK, resp.StatusCode)
assert.Equal(t, "Status: OK\n", string(body))

// Clean up
cleanup()
// Stop the supervisor
sv.Shutdown()

// Check that Run() didn't return an error
select {
Expand Down Expand Up @@ -88,7 +87,6 @@ func TestRunServerInvalidPort(t *testing.T) {

// Create HTTP server runner with invalid port
runner, err := httpserver.NewRunner(
httpserver.WithContext(ctx),
httpserver.WithConfigCallback(configCallback),
httpserver.WithLogHandler(logHandler.WithGroup("httpserver")),
)
Expand Down
1 change: 0 additions & 1 deletion runnables/httpcluster/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ func defaultRunnerFactory(
return httpserver.NewRunner(
httpserver.WithName(id),
httpserver.WithConfig(cfg),
httpserver.WithContext(ctx),
httpserver.WithLogHandler(handler),
)
}
Expand Down
10 changes: 4 additions & 6 deletions runnables/httpserver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,10 @@ func TestContextPropagation(t *testing.T) {
return NewConfig(listenPort, hConfig, WithDrainTimeout(2*time.Second))
}

// Create a new context that we'll cancel to trigger shutdown
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
// Context for the server Run method
ctx := t.Context()

server, err := NewRunner(
WithContext(ctx),
WithConfigCallback(cfgCallback),
)
require.NoError(t, err)
Expand All @@ -313,7 +311,7 @@ func TestContextPropagation(t *testing.T) {

// Start the server in a goroutine
go func() {
err := server.Run(context.Background())
err := server.Run(ctx)
runComplete <- err
}()

Expand Down Expand Up @@ -346,7 +344,7 @@ func TestContextPropagation(t *testing.T) {
}

// Initiate server shutdown
cancel() // This should cancel the context passed to the server
server.Stop() // This should cancel the server's context

// Verify that the handler's context was canceled
select {
Expand Down
3 changes: 1 addition & 2 deletions runnables/httpserver/helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package httpserver

import (
"context"
"fmt"
"net"
"net/http"
Expand Down Expand Up @@ -44,7 +43,7 @@ func createTestServer(
return NewConfig(listenPort, hConfig, WithDrainTimeout(drainTimeout))
}

server, err := NewRunner(WithContext(context.Background()), WithConfigCallback(cfgCallback))
server, err := NewRunner(WithConfigCallback(cfgCallback))
require.NoError(t, err)
require.NotNil(t, server)

Expand Down
11 changes: 0 additions & 11 deletions runnables/httpserver/options.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package httpserver

import (
"context"
"log/slog"
)

Expand All @@ -21,16 +20,6 @@ func WithLogHandler(handler slog.Handler) Option {
}
}

// WithContext sets a custom context for the Runner instance.
// This allows for more granular control over cancellation and timeouts.
func WithContext(ctx context.Context) Option {
return func(r *Runner) {
if ctx != nil {
r.ctx, r.cancel = context.WithCancel(ctx)
}
}
}

// WithConfigCallback sets the function that will be called to load or reload configuration.
// Either this option or WithConfig initializes the Runner instance by providing the
// configuration for the HTTP server managed by the Runner.
Expand Down
45 changes: 0 additions & 45 deletions runnables/httpserver/options_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package httpserver

import (
"context"
"log/slog"
"net/http"
"strings"
Expand All @@ -13,47 +12,6 @@ import (
"github.com/stretchr/testify/require"
)

// Define a custom type for context keys to avoid string collision
type contextKey string

// TestWithContext verifies the WithContext option works correctly
func TestWithContext(t *testing.T) {
t.Parallel()
// Create a custom context with a value using the type-safe key
testKey := contextKey("test-key")
customCtx := context.WithValue(context.Background(), testKey, "test-value")
// Create a server with the custom context
handler := func(w http.ResponseWriter, r *http.Request) {}
route, err := NewRoute("v1", "/", handler)
require.NoError(t, err)
hConfig := Routes{*route}
cfgCallback := func() (*Config, error) {
return NewConfig(":0", hConfig, WithDrainTimeout(1*time.Second))
}
server, err := NewRunner(WithContext(context.Background()),
WithConfigCallback(cfgCallback),
WithContext(customCtx))
require.NoError(t, err)
// Verify the custom context was applied
actualValue := server.ctx.Value(testKey)
assert.Equal(t, "test-value", actualValue, "Context value should be preserved")
// Verify cancellation works through server.Stop()
done := make(chan struct{})
go func() {
<-server.ctx.Done()
close(done)
}()
// Call Stop to cancel the internal context
server.Stop()
// Wait for the server context to be canceled or timeout
select {
case <-done:
// Success, context was canceled
case <-time.After(1 * time.Second):
t.Fatal("Context cancellation not propagated")
}
}

func TestWithLogHandler(t *testing.T) {
t.Parallel()
// Create a custom logger with a buffer for testing output
Expand All @@ -69,7 +27,6 @@ func TestWithLogHandler(t *testing.T) {
}
// Create a server with the custom logger
server, err := NewRunner(
WithContext(context.Background()),
WithConfigCallback(cfgCallback),
WithLogHandler(customHandler),
)
Expand All @@ -95,7 +52,6 @@ func TestWithConfig(t *testing.T) {
require.NoError(t, err)
// Create a server with the static config
server, err := NewRunner(
WithContext(context.Background()),
WithConfig(staticConfig),
)
require.NoError(t, err)
Expand Down Expand Up @@ -143,7 +99,6 @@ func TestWithServerCreator(t *testing.T) {
}
// Create a server with the config that has a custom server creator
server, err := NewRunner(
WithContext(context.Background()),
WithConfigCallback(cfgCallback),
)
require.NoError(t, err)
Expand Down
2 changes: 0 additions & 2 deletions runnables/httpserver/reload_mocked_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package httpserver

import (
"context"
"errors"
"net/http"
"testing"
Expand Down Expand Up @@ -203,7 +202,6 @@ func TestReloadConfig_WithFullRunner(t *testing.T) {

// Create the Runner with the config callback
runner, err := NewRunner(
WithContext(context.Background()),
WithConfigCallback(configCallback),
)
require.NoError(t, err)
Expand Down
Loading