danielgtaylor / huma

Huma REST/HTTP API Framework for Golang with OpenAPI 3.1
https://huma.rocks/
MIT License
1.89k stars 138 forks source link

huma panics on client context canceled #529

Open nunoo opened 1 month ago

nunoo commented 1 month ago

On line huma.go:483, if the client cancels the context, it results in a panic. This explicitly removes control flow from the user, and actually ends up writing out a status 500 in any lower level middlewares (like an http.Handler middleware receives a 500, instead of the 499 it should've been, had we propagated the context canceled error)

danielgtaylor commented 1 month ago

@nunoo do you mean these lines? https://github.com/danielgtaylor/huma/blob/main/huma.go#L487-L491. This happens after the handler has already returned the response struct, so there's no way to pass the error back to the handler. We could try to catch the context canceled scenario and set the response status appropriately to 499. How were you thinking this should be solved?

nunoo commented 1 month ago

This is our stack trace:

/usr/local/go/src/runtime/panic.go:770 +0x132
github.com/danielgtaylor/huma/v2.transformAndWrite({0x15cbbf8, 0xc000459700}, {0x15df778, 0xc000345e00}, 0x1f3, {0xc000b70330, 0x10}, {0x12e3580, 0xc0009ca708})
    /go/pkg/mod/github.com/danielgtaylor/huma/v2@v2.18.0/huma.go:483 +0x33b
github.com/danielgtaylor/huma/v2.Register[...].func1()
    /go/pkg/mod/github.com/danielgtaylor/huma/v2@v2.18.0/huma.go:1349 +0x12b3
github.com/danielgtaylor/huma/v2/adapters/humago.(*goAdapter).Handle.func1({0x15c13e0, 0xc000d786a0}, 0xc000a8cc60)
    /go/pkg/mod/github.com/danielgtaylor/huma/v2@v2.18.0/adapters/humago/humago.go:132 +0xa2
net/http.HandlerFunc.ServeHTTP(0x15c34a8?, {0x15c13e0?, 0xc000d786a0?}, 0x15a95c0?)
    /usr/local/go/src/net/http/server.go:2171 +0x29

Although looking at the code, maybe we have this inverted, and there's a panic due to something else, which in turn cancels the context. FYI we're only using the DefaultConfig

danielgtaylor commented 1 month ago

@nunoo do you have a way to reproduce this issue? I can't seem to trigger the panic. I'v modified the greet example like this:

package main

import (
    "context"
    "fmt"
    "net/http"
    "time"

    "github.com/danielgtaylor/huma/v2"
    "github.com/danielgtaylor/huma/v2/adapters/humago"
    "github.com/danielgtaylor/huma/v2/humacli"

    _ "github.com/danielgtaylor/huma/v2/formats/cbor"
)

// Options for the CLI.
type Options struct {
    Port int `help:"Port to listen on" short:"p" default:"8888"`
}

// GreetingInput represents the greeting operation request.
type GreetingInput struct {
    Name string `path:"name" maxLength:"30" example:"world" doc:"Name to greet"`
}

// GreetingOutput represents the greeting operation response.
type GreetingOutput struct {
    Body struct {
        Message string `json:"message" example:"Hello, world!" doc:"Greeting message"`
    }
}

func main() {
    // Create a CLI app which takes a port option.
    cli := humacli.New(func(hooks humacli.Hooks, options *Options) {
        // Create a new router & API
        router := http.NewServeMux()

        mw := func(h http.Handler) http.Handler {
            return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                start := time.Now()
                h.ServeHTTP(w, r)
                fmt.Printf("%s %s %s %s\n", r.Method, r.URL.Path, r.RemoteAddr, time.Since(start))
                fmt.Println("middleware done")
            })
        }

        api := humago.New(router, huma.DefaultConfig("My API", "1.0.0"))

        // Register GET /greeting/{name}
        huma.Register(api, huma.Operation{
            OperationID: "get-greeting",
            Summary:     "Get a greeting",
            Method:      http.MethodGet,
            Path:        "/greeting/{name}",
        }, func(ctx context.Context, input *GreetingInput) (*GreetingOutput, error) {
            resp := &GreetingOutput{}
            resp.Body.Message = fmt.Sprintf("Hello, %s!", input.Name)
            time.Sleep(5 * time.Second)
            return resp, nil
        })

        // Tell the CLI how to start your router.
        hooks.OnStart(func() {
            http.ListenAndServe(fmt.Sprintf(":%d", options.Port), mw(router))
        })
    })

    // Run the CLI. When passed no commands, it starts the server.
    cli.Run()
}

Then I run it, and use restish or curl to hit the URL (e.g. curl localhost:8888/greeting/test) and ctrl-c to disconnect before 5 seconds, but the server never panics. Do you have a simple way to repro?

nunoo commented 1 month ago

@danielgtaylor thanks for taking a deeper look into this! The error I was seeing may have been related to the fix in #542 so I'll wait until that is released to see if the issue persists