Skip to content
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

Drop context arg from HTTP handler #58

Merged
merged 2 commits into from
Mar 14, 2024
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
4 changes: 2 additions & 2 deletions cloudevents/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func New(f any) *Service {
mux.HandleFunc("/health/readiness", svc.Ready)
mux.HandleFunc("/health/liveness", svc.Alive)
mux.Handle("/", newCloudeventHandler(f)) // See implementation note
svc.Server.Handler = mux
svc.Handler = mux
return svc
}

Expand Down Expand Up @@ -91,7 +91,7 @@ func (s *Service) Start(ctx context.Context) (err error) {
s.handleSignals()

go func() {
if err = s.Server.Serve(s.listener); err != http.ErrServerClosed {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data race was here

if err := s.Serve(s.listener); err != http.ErrServerClosed {
log.Error().Err(err).Msg("http server exited with unexpected error")
s.stop <- err
}
Expand Down
9 changes: 4 additions & 5 deletions cmd/fhttp/main.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package main

import (
"context"
"fmt"
"net/http"
"os"
Expand All @@ -25,9 +24,9 @@ func main() {
}

// Example Static HTTP Handler implementation.
func Handle(ctx context.Context, res http.ResponseWriter, req *http.Request) {
func Handle(w http.ResponseWriter, r *http.Request) {
fmt.Println("Static HTTP handler invoked")
res.Write([]byte("Static HTTP handler invoked\n"))
fmt.Fprintln(w, "Static HTTP Handler invoked")
}

// MyFunction is an example instanced HTTP function implementation.
Expand All @@ -37,7 +36,7 @@ func New() *MyFunction {
return &MyFunction{}
}

func (f *MyFunction) Handle(_ context.Context, res http.ResponseWriter, req *http.Request) {
func (f *MyFunction) Handle(w http.ResponseWriter, r *http.Request) {
fmt.Println("Instanced HTTP handler invoked")
res.Write([]byte("Instanced HTTP handler invoked\n"))
fmt.Fprintln(w, "Instanced HTTP Handler invoked")
}
24 changes: 5 additions & 19 deletions http/instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package http
import (
"context"
"net/http"

"github.com/rs/zerolog/log"
)

// Handler is a function instance which can handle a request.
Expand All @@ -14,9 +12,11 @@ import (
// framework is the same.
type Handler interface {
// Handle a request.
Handle(context.Context, http.ResponseWriter, *http.Request)
Handle(http.ResponseWriter, *http.Request)
}

type HandleFunc func(http.ResponseWriter, *http.Request)

// Starter is an instance which has defined the Start hook
type Starter interface {
// Start instance event hook.
Expand All @@ -41,26 +41,12 @@ type LivenessReporter interface {
Alive(context.Context) (bool, error)
}

// HandleFunc defines the function signature expected of static Functions.
type HandleFunc func(context.Context, http.ResponseWriter, *http.Request)

// DefaultHandler is used for simple static function implementations which
// need only define a single exported function named Handle of type HandleFunc.
type DefaultHandler struct {
Handler HandleFunc
}

// Handle a request by passing to the handler function.
func (f DefaultHandler) Handle(ctx context.Context, res http.ResponseWriter, req *http.Request) {
if f.Handler == nil {
f.Handler = defaultHandler
}
f.Handler(ctx, res, req)
}

// DefaultHandler is a Handler implementation which simply warns the user that
// the default handler instance was not properly initialized.
var defaultHandler = func(_ context.Context, res http.ResponseWriter, req *http.Request) {
log.Warn().Msg("no Handle method found")
http.NotFound(res, req)
func (f DefaultHandler) Handle(w http.ResponseWriter, r *http.Request) {
f.Handler(w, r)
}
6 changes: 3 additions & 3 deletions http/mock/function.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
type Function struct {
OnStart func(context.Context, map[string]string) error
OnStop func(context.Context) error
OnHandle func(context.Context, http.ResponseWriter, *http.Request)
OnHandle func(http.ResponseWriter, *http.Request)
}

func (f *Function) Start(ctx context.Context, cfg map[string]string) error {
Expand All @@ -25,8 +25,8 @@ func (f *Function) Stop(ctx context.Context) error {
return nil
}

func (f *Function) Handle(ctx context.Context, w http.ResponseWriter, r *http.Request) {
func (f *Function) Handle(w http.ResponseWriter, r *http.Request) {
if f.OnHandle != nil {
f.OnHandle(ctx, w, r)
f.OnHandle(w, r)
}
}
29 changes: 10 additions & 19 deletions http/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const (
// Start an intance using a new Service
// Note that for CloudEvent Handlers this effectively accepts ANY because
// the actual type of the handler function is determined later.
func Start(f any) error {
func Start(f Handler) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this - since we want a proper Handler to be passed here.

We shouldn't be failing this during runtime - it will swallow errors.

Copy link
Contributor

@lkingland lkingland Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I thought so too. But then thought it would be possible one might want a Function which fires "OnStart" and "OnStop" but does not accept HTTP requests, or is set to never scale to 0 and performs some action on a chron-like timer. This would open us up to having Functions which fill very different roles than the expected, request-response HTTP cycle... and I think that's a good thing. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would open us up to having Functions which fill very different roles than the expected, request-response HTTP cycle

For these use cases I'd use a different template and not the HTTP one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondly, if the user typos Handle then they won't get feedback here because it's loosely typed.

log.Debug().Msg("func runtime creating function instance")
return New(f).Start(context.Background())
}
Expand All @@ -42,11 +42,11 @@ type Service struct {
http.Server
listener net.Listener
stop chan error
f any
f Handler
}

// New Service which serves the given instance.
func New(f any) *Service {
func New(f Handler) *Service {
svc := &Service{
f: f,
stop: make(chan error),
Expand All @@ -62,7 +62,7 @@ func New(f any) *Service {
mux.HandleFunc("/health/readiness", svc.Ready)
mux.HandleFunc("/health/liveness", svc.Alive)
mux.HandleFunc("/", svc.Handle)
svc.Server.Handler = mux
svc.Handler = mux

// Print some helpful information about which interfaces the function
// is correctly implementing
Expand All @@ -74,9 +74,6 @@ func New(f any) *Service {
// log which interfaces the function implements.
// This could be more verbose for new users:
func logImplements(f any) {
if _, ok := f.(Handler); ok {
log.Info().Msg("Function implements Handle")
}
if _, ok := f.(Starter); ok {
log.Info().Msg("Function implements Start")
}
Expand Down Expand Up @@ -123,7 +120,7 @@ func (s *Service) Start(ctx context.Context) (err error) {

// Listen and serve
go func() {
if err = s.Server.Serve(s.listener); err != http.ErrServerClosed {
if err := s.Serve(s.listener); err != http.ErrServerClosed {
Copy link
Contributor Author

@dprotaso dprotaso Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data race was here

log.Error().Err(err).Msg("http server exited with unexpected error")
s.stop <- err
}
Expand Down Expand Up @@ -180,13 +177,7 @@ func (s *Service) Addr() net.Addr {

// Handle requests for the instance
func (s *Service) Handle(w http.ResponseWriter, r *http.Request) {
if i, ok := s.f.(Handler); ok {
i.Handle(r.Context(), w, r)
} else {
message := "function does not implement Handle. Skipping invocation."
log.Debug().Msg(message)
_, _ = w.Write([]byte(message))
}
s.f.Handle(w, r)
}

// Ready handles readiness checks.
Expand All @@ -197,11 +188,11 @@ func (s *Service) Ready(w http.ResponseWriter, r *http.Request) {
message := "error checking readiness"
log.Debug().Err(err).Msg(message)
w.WriteHeader(500)
_, _ = w.Write([]byte(message + ". " + err.Error()))
fmt.Fprint(w, "error checking readiness: ", err.Error())
return
}
if !ready {
message := "function not yet available"
message := "function not yet ready"
log.Debug().Msg(message)
w.WriteHeader(503)
fmt.Fprintln(w, message)
Expand All @@ -219,11 +210,11 @@ func (s *Service) Alive(w http.ResponseWriter, r *http.Request) {
message := "error checking liveness"
log.Err(err).Msg(message)
w.WriteHeader(500)
_, _ = w.Write([]byte(message + ". " + err.Error()))
fmt.Fprint(w, "error checking liveness: ", err.Error())
return
}
if !alive {
message := "function not ready"
message := "function not alive"
log.Debug().Msg(message)
w.WriteHeader(503)
_, _ = w.Write([]byte(message))
Expand Down
4 changes: 2 additions & 2 deletions http/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func TestHandle_Invoked(t *testing.T) {
startCh <- true
return nil
}
onHandle = func(_ context.Context, w http.ResponseWriter, _ *http.Request) {
onHandle = func(w http.ResponseWriter, _ *http.Request) {
fmt.Fprintf(w, "OK")
}
)
Expand Down Expand Up @@ -358,7 +358,7 @@ func TestAlive_Invoked(t *testing.T) {
errCh = make(chan error)
startCh = make(chan any)
timeoutCh = time.After(500 * time.Millisecond)
onStart = func(_ context.Context, _ map[string]string) error {
onStart = func(context.Context, map[string]string) error {
startCh <- true
return nil
}
Expand Down
Loading