Skip to content

otelhttp.NewHandler body wrapper alters default behaviour of Expect: 100-continue header handling #6908

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

Open
VirrageS opened this issue Mar 10, 2025 · 2 comments · May be fixed by #6914
Open
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp

Comments

@VirrageS
Copy link

VirrageS commented Mar 10, 2025

Description

otelhttp.NewHandler adds extra body wrapper on top of r.Body: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/net/http/otelhttp/handler.go#L138-L144. This changes default behavior of handling Expect: 100-continue header by Go HTTP server.

Here: https://github.com/golang/go/blob/go1.24.1/src/net/http/server.go#L1418-L1420 since r.Body is not the expectContinueReader (it is our custom wrapper) this means that we won't trigger close after reply so instead of that we will go here: https://github.com/golang/go/blob/go1.24.1/src/net/http/server.go#L1461-L1462 and we will wait for the body without sending 100 Continue response back (which means that client won't send us data until the specified timeout and thus whole operations takes seconds instead of milliseconds).

Environment

  • OS: MacOS
  • Architecture: arch64
  • Go Version: 1.24
  • otelhttp version: v0.56.0

Steps To Reproduce

  1. This is toy example on the problem itself:
    package main
    
    import (
    	"bytes"
    	"crypto/rand"
    	"fmt"
    	"io"
    	"net/http"
    	"time"
    )
    
    type customBodyWrapper struct {
    	rc io.ReadCloser
    }
    
    func (cbw customBodyWrapper) Read(p []byte) (n int, err error) {
    	return cbw.rc.Read(p)
    }
    
    func (cbw customBodyWrapper) Close() error {
    	return cbw.rc.Close()
    }
    
    func startRedirectServer() {
    	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    		r.Body = customBodyWrapper{rc: r.Body} // <- THIS IS THE PROBLEM. REMOVING THIS FIXES IT.
    		http.Redirect(w, r, "http://localhost:8082/target", http.StatusTemporaryRedirect)
    	})
    
    	fmt.Println("Redirect server listening on :8081")
    	go func() {
    		http.ListenAndServe(":8081", nil)
    	}()
    }
    
    func startTargetServer() {
    	http.HandleFunc("/target", func(w http.ResponseWriter, r *http.Request) {
    		body, _ := io.ReadAll(r.Body)
    		w.Write(body)
    	})
    
    	fmt.Println("Target server listening on :8082")
    	go func() {
    		http.ListenAndServe(":8082", nil)
    	}()
    }
    
    func sendClientRequest() {
    	client := &http.Client{
    		Transport: &http.Transport{
    			ExpectContinueTimeout: 5 * time.Second,
    		},
    	}
    
    	body := make([]byte, 5*1024*1024)
    	rand.Read(body)
    	
    	req, _ := http.NewRequest("PUT", "http://localhost:8081/", bytes.NewReader(body))
    	req.Header.Set("Expect", "100-continue")
    	req.ContentLength = int64(len(body))
    
    	resp, _ := client.Do(req)
    	defer resp.Body.Close()
    	responseBody, _ := io.ReadAll(resp.Body)
    	fmt.Printf("Code: %d, Response: %d\n", resp.StatusCode, len(responseBody))
    }
    
    func main() {
    	startRedirectServer()
    	startTargetServer()
    	time.Sleep(time.Second)
    	fmt.Println("")
    
    	sendClientRequest()
    }
  2. Run this file.
  3. See how long it takes with and without custom wrapper.

Expected behavior

No extra delay when handling Expect: 100-continue headers during redirect (without reading the r.Body).

Further notes

In my opinion Go handling of this header is not ideal. Expecting specific type might not be a good idea... Not sure if we should instead file a ticket in Go to improve this...

... but in the meantime one of the thing that otelhttp library could do is something like:

	bw := request.NewBodyWrapper(r.Body, readRecordFunc)
	if r.Body != nil && r.Body != http.NoBody {
+		prevBody := r.Body
		r.Body = bw
+		defer func() { r.Body = prevBody }
	}

But not sure if this actually works properly and if it doesn't breaking something else instead :D

@VirrageS VirrageS added area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp labels Mar 10, 2025
@dmathieu
Copy link
Member

Thinking about this, I'm not seeing any side effects doing this could have, and your issue does make sense.
Do you want to open a PR with this fix and tests?

@VirrageS
Copy link
Author

I could try - I should be able to find some time during the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp
Projects
Status: Low priority
Development

Successfully merging a pull request may close this issue.

2 participants