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

feat: allow ReadTimeout for http.Server to be configured #194

Open
lipangeng opened this issue Dec 22, 2024 · 5 comments
Open

feat: allow ReadTimeout for http.Server to be configured #194

lipangeng opened this issue Dec 22, 2024 · 5 comments

Comments

@lipangeng
Copy link

lipangeng commented Dec 22, 2024

It should be possible for users to override these settings when deploying the public mccutchn/go-httpbin Docker image without having to build and deploy their own customized image:

// Reasonable defaults for our http server
srvReadTimeout = 5 * time.Second
srvReadHeaderTimeout = 1 * time.Second
srvMaxHeaderBytes = 16 * 1024 // 16kb

Those settings are then used here:

srv := &http.Server{
Addr: net.JoinHostPort(cfg.ListenHost, strconv.Itoa(cfg.ListenPort)),
Handler: app.Handler(),
MaxHeaderBytes: srvMaxHeaderBytes,
ReadHeaderTimeout: srvReadHeaderTimeout,
ReadTimeout: srvReadTimeout,
}


Original issue description When uploading files, it usually takes a long time. The default 5s time is not enough. Adjust to configurable items for greater flexibility.
@mccutchen
Copy link
Owner

mccutchen commented Dec 22, 2024

As noted in the Configuration section of the README, the maximum request duration can be configured via the -max-duration command line argument or the MAX_DURATION environment variable, and you can obviously set whatever ReadTimeout value is appropriate on the http.Server instance used to serve traffic. Please adjust as necessary in your own deployment of go-httpbin!

I am unlikely to change this configuration for the public instance of go-httpbin available at https://httpgingo.org.

@mccutchen mccutchen closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2024
@mccutchen
Copy link
Owner

In case it's helpful, the configuration for that public https://httpbingo.org instance can be seen here:

	h := httpbin.New(
		httpbin.WithMaxBodySize(maxBodySize),
		httpbin.WithMaxDuration(maxDuration),
		httpbin.WithHostname(hostname),
		httpbin.WithAllowedRedirectDomains(allowedRedirectDomains),
		httpbin.WithExcludeHeaders(strings.Join(excludedHeaders, ",")),
	)

	var handler http.Handler
	handler = h.Handler()
	handler = spamFilter(handler)
	handler = hlog.AccessHandler(requestLogger)(handler)
	handler = hlog.NewHandler(logger)(handler)

	srv := &http.Server{
		Addr:    fmt.Sprintf("0.0.0.0:%s", os.Getenv("PORT")),
		Handler: handler,

		ReadTimeout:       2 * time.Second,
		ReadHeaderTimeout: 1 * time.Second,
		MaxHeaderBytes:    1024 * 4, // 4kb
	}

@lipangeng
Copy link
Author

Httpbin is excellent, and I'm using my own deployment. the ReadTimeout of the http.Server is hard-coded, requiring modification of the source code, recompilation, and rebuilding of the image. I think it would be more user-friendly if these parameters were configurable.

Now it's like this.

// Reasonable defaults for our http server
	srvReadTimeout       = 5 * time.Second
	srvReadHeaderTimeout = 1 * time.Second
	srvMaxHeaderBytes    = 16 * 1024 // 16kb

srv := &http.Server{
		Addr:              net.JoinHostPort(cfg.ListenHost, strconv.Itoa(cfg.ListenPort)),
		Handler:           app.Handler(),
		MaxHeaderBytes:    srvMaxHeaderBytes,
		ReadHeaderTimeout: srvReadHeaderTimeout,
		ReadTimeout:       srvReadTimeout,
	}

@mccutchen
Copy link
Owner

Ah, okay, thank you for clarifying, I closed this too quickly!

I’d be happy to accept a change to make those parameters configurable via CLI arguments and environment variables, if you or anyone else wants to work on that.

@mccutchen mccutchen reopened this Dec 24, 2024
@mccutchen mccutchen changed the title feat: Allows ReadTimeout for http.Server to be configured feat: allow ReadTimeout for http.Server to be configured Dec 27, 2024
@mccutchen
Copy link
Owner

(I updated the issue description above to be a bit more specific, in case it's helpful for anyone who might want to tackle this feature request!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants