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

Merged
merged 2 commits into from
Mar 28, 2025

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

@ldez ldez marked this pull request as draft December 16, 2024 18:01
@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.

@adamvduke adamvduke marked this pull request as ready for review December 30, 2024 02:33
@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?

@lbenguigui
Copy link

@lbenguigui can you evaluate this PR inside Traefik?

Sure

@lbenguigui
Copy link

@ldez I tested the PR with a Traefik setup, and it works fine. Let me know if you need any specific feedback.

@ldez ldez force-pushed the adamvduke/add-buffer-limit-test-cases branch from 0cdfa5c to 1ab50f5 Compare March 28, 2025 22:56
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit 19a97c8 into vulcand:master Mar 28, 2025
5 checks passed
@adamvduke adamvduke deleted the adamvduke/add-buffer-limit-test-cases branch March 29, 2025 19:52
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.

3 participants