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

huma panics on client context canceled #529

Closed
nunoo opened this issue Jul 30, 2024 · 5 comments
Closed

huma panics on client context canceled #529

nunoo opened this issue Jul 30, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@nunoo
Copy link
Contributor

nunoo commented Jul 30, 2024

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 danielgtaylor added the bug Something isn't working label Jul 30, 2024
@danielgtaylor
Copy link
Owner

@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
Copy link
Contributor Author

nunoo commented Jul 30, 2024

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
Copy link
Owner

danielgtaylor commented Aug 16, 2024

@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
Copy link
Contributor Author

nunoo commented Aug 16, 2024

@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

@danielgtaylor
Copy link
Owner

This should also now be fixed in #650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants