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

[BUG] Support for http.ResponseController timeouts in WrapHandler #2674

Closed
chrisforrette opened this issue Apr 26, 2024 · 2 comments · Fixed by #2775
Closed

[BUG] Support for http.ResponseController timeouts in WrapHandler #2674

chrisforrette opened this issue Apr 26, 2024 · 2 comments · Fixed by #2775
Assignees
Labels
bug unintended behavior that has to be fixed

Comments

@chrisforrette
Copy link

chrisforrette commented Apr 26, 2024

This is somewhere between a bug and a feature request as this is to accommodate relatively new (as of version 1.20) functionality in Go in a contrib package.

Version of dd-trace-go

v1.62.0

Describe what happened:

The issue is with the net/http contrib/ package, specifically with the WrapHandler function.

We were attempting to use http.ResponseController to add read and write timeouts to our HTTP handlers, roughly like so:

import (
	"context"
	"net/http"
	"time"
	datadog "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
)

// We use something like this to wrap all of our HTTP handlers (it's old, don't judge me)
func WrapHandler(ctx context.Context, handlerConfig *HandlerConfig) http.Handler {
	fn := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		// ...
		rc := http.NewResponseController(w)
		err := rc.SetReadDeadline(time.Now().Add(handlerConfig.ReadTimeout))
		if err != nil {
			logger.WithError(err).Error("error setting handler read deadline")
		}
		err = rc.SetWriteDeadline(time.Now().Add(handlerConfig.WriteTimeout))
		if err != nil {
			logger.WithError(err).Error("error setting handler write deadline")
		}
		//	...
		return handlerConfig.Handler(w, r)
	})
	return datadog.WrapHandler(fn, "service-name", "resource-name")
}

But when those endpoints are hit, we see an internal-to-Go error: feature not supported

I was able to trace the issue like so:

  1. WrapHandler calls TraceAndServe here: https://github.com/DataDog/dd-trace-go/blob/main/contrib/net/http/http.go#L91
  2. TraceAndServe passes the http.ResponseWriter object to wrapResponseWriter, overwriting it with the result here: https://github.com/DataDog/dd-trace-go/blob/main/contrib/net/http/trace.go#L68
  3. wrapResponseWriter then tries to check the object to see if it matches a bunch of common http interfaces like Flusher, Pusher, Hijacker, etc., and creates a new struct on the fly, making a new instance of it passing in the original http.ResponseWriter object.

It's the last step where I think the problem is. http.ResponseController tries to call SetReadDeadline and SetWriteDeadline on the http.ResponseWriter object but that doesn't match any of the interfaces wrapResponseWriter is coercing the object into so those methods apparently seem to get dropped from it.

Describe what you expected:

I can use http.ResponseController with SetReadDeadline and SetWriteDeadline methods to set read/write timeouts on an HTTP handler with dd-trace-go's WrapHandler.

Steps to reproduce the issue:

(described above)

Additional environment details (Version of Go, Operating System, etc.):

Go version: 1.22.1

@chrisforrette chrisforrette added the bug unintended behavior that has to be fixed label Apr 26, 2024
@github-actions github-actions bot added the needs-triage New issues that have not yet been triaged label Apr 26, 2024
@darccio darccio removed the needs-triage New issues that have not yet been triaged label Apr 30, 2024
@zarirhamza
Copy link
Contributor

Hey @chrisforrette! After taking a closer look it seems that the http.ResponseController code was updated to include new methods (SetReadDeadline and SetWriteDeadline) after our initial implementation

We will work on a fix as soon as we can but feel free to open your own PR if you'd like. Regardless, this should be fixed in the near future.

@rarguelloF
Copy link
Contributor

This issue has been fixed in #2775 and it should be included in v1.66.0 (it is expected to be released in July).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants