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

buffer: handle bufferWriter error #247

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

adamvduke
Copy link
Contributor

@adamvduke adamvduke commented Dec 16, 2024

When looking into traefik/traefik#10687, I came across some interesting behavior where the buffer changes from returning a 200 to a 500 depending on the configured limits, but then also changes back and forth when crossing a size equal to a multiple of 32768 bytes.

For instance, the traefik issue configures a buffer that only sets the maxResponseBodyBytes option to a value of 2000 bytes. For response sizes from 1 up to 2000 bytes, the buffer behaves correctly and returns a 200 status which is expected. From 2001 up to 32768, the buffer returns a 500 status which is also expected. From 32769 up to 34769 (32769 + 2000) the buffer changes back to returning a 200 status, which I don't think is the expected behavior. From 34769 up to 65538 the behavior changes back to a 500, and so on at size boundaries equal to multiples of 32768.

$ go test -v ./buffer -count=1 -run TestBuffer_responseLimitReached
=== RUN   TestBuffer_responseLimitReached
=== RUN   TestBuffer_responseLimitReached/small_limit_with_body_larger_than_max_response_bytes
=== RUN   TestBuffer_responseLimitReached/small_limit_with_body_larger_than_32768_bytes
    buffer_test.go:243: 
        	Error Trace:	/Users/adamd/go/src/github.com/adamvduke/oxy/buffer/buffer_test.go:243
        	Error:      	Not equal: 
        	            	expected: 500
        	            	actual  : 200
        	Test:       	TestBuffer_responseLimitReached/small_limit_with_body_larger_than_32768_bytes
=== RUN   TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_bytes
    buffer_test.go:243: 
        	Error Trace:	/Users/adamd/go/src/github.com/adamvduke/oxy/buffer/buffer_test.go:243
        	Error:      	Not equal: 
        	            	expected: 500
        	            	actual  : 200
        	Test:       	TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_bytes
=== RUN   TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_+_1999_bytes
    buffer_test.go:243: 
        	Error Trace:	/Users/adamd/go/src/github.com/adamvduke/oxy/buffer/buffer_test.go:243
        	Error:      	Not equal: 
        	            	expected: 500
        	            	actual  : 200
        	Test:       	TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_+_1999_bytes
=== RUN   TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_+_2000_bytes
=== RUN   TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_65536_+_1999_bytes
    buffer_test.go:243: 
        	Error Trace:	/Users/adamd/go/src/github.com/adamvduke/oxy/buffer/buffer_test.go:243
        	Error:      	Not equal: 
        	            	expected: 500
        	            	actual  : 200
        	Test:       	TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_65536_+_1999_bytes
=== RUN   TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_65536_+_2000_bytes
--- FAIL: TestBuffer_responseLimitReached (0.01s)
    --- PASS: TestBuffer_responseLimitReached/small_limit_with_body_larger_than_max_response_bytes (0.00s)
    --- FAIL: TestBuffer_responseLimitReached/small_limit_with_body_larger_than_32768_bytes (0.00s)
    --- FAIL: TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_bytes (0.00s)
    --- FAIL: TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_+_1999_bytes (0.00s)
    --- PASS: TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_32768_+_2000_bytes (0.00s)
    --- FAIL: TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_65536_+_1999_bytes (0.00s)
    --- PASS: TestBuffer_responseLimitReached/larger_limit_with_body_larger_than_65536_+_2000_bytes (0.00s)
FAIL
FAIL	github.com/vulcand/oxy/v2/buffer	0.643s
FAIL

@adamvduke adamvduke force-pushed the adamvduke/add-buffer-limit-test-cases branch from 9bbed94 to dfdf4a4 Compare December 30, 2024 02:31
@adamvduke
Copy link
Contributor Author

I'm not sure I have enough context to know that this approach is correct. It does seem like the error when writing to the buffer should propagate back to the client rather than logging it and moving on, and there is an existing pattern of using b.errHandler.ServeHTTP(w, req, err) to propagate other error conditions.

@ldez ldez changed the title add additional buffer limit test cases buffer: handle bufferWriter error Mar 21, 2025
@ldez ldez added the bug label Mar 21, 2025
@ldez ldez self-requested a review March 21, 2025 16:11
@ldez
Copy link
Member

ldez commented Mar 21, 2025

@lbenguigui can you evaluate this PR inside Traefik?

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

Successfully merging this pull request may close these issues.

2 participants