DataDog/dd-trace-go

[BUG] Support for http.ResponseController timeouts in WrapHandler

Closed this issue · 2 comments

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

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.

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