Skip to content
This repository was archived by the owner on Jul 29, 2025. It is now read-only.

Conversation

hiyosi
Copy link

@hiyosi hiyosi commented Sep 10, 2024

Description

There are some bugs in the buffer pool implementation of the infra package.

  1. Unnecessary clean slice.

https://github.com/AthenZ/authorization-proxy/blob/4bc56bbbf670d016cd84167662c26a674a959953/infra/buffer.go#L64

The copyBuffer function in the httputil.ReverseProxy package checks the length of the slice that given as argument(buf), and if the length of the slice is 0, it makes a slice as 32 * 1024 (32KB).
The authorisation-proxy (infra package) always reset the slice length to 0 before put it to the pool, this means that the buffer size is always 32KB when copying.

  1. Incorrect comparison conditions.

https://github.com/AthenZ/authorization-proxy/blob/4bc56bbbf670d016cd84167662c26a674a959953/infra/buffer.go#L74

Even if a slice with the same capacity as buffer.size is put, a new slice will be created.
Since httputil.ReverseProxy does not change the slice size, results in memory allocation every time a response is copied.
The impact of this issue is limited, you probably can't notice it if you don't specify a large value for bufferSize (see result of memory usage).

In this PR, a new simpleBuffer is introduced to solve the above problem.
The current implementation of BufferPool is overkill because the httputil.Reverseproxy does not resize the slice used to copy the response.
However the current implementation is still exists, it could be used for other purposes.

e.g. memory usage (origin server returns small response)

before

bufferSize requests/sec duration max rss (kb)
419430400 1/s 1s 440288
419430400 1/s 10s 1669664
419430400 100/s 1s 18902020
419430400 100/s 10s 27421440

after

bufferSize requests/sec duration max rss (kb)
419430400 1/s 1s 435296
419430400 1/s 10s 441516
419430400 100/s 1s 3351424
419430400 100/s 10s 6216764

Type of change

  • Bug fix
  • New feature
  • Refactoring (no functional changes, no api changes)
  • Non-code changes (update documentation, pipeline, etc.)

Flags

  • Breaks backward compatibility
  • Requires a documentation update
  • Has untestable code

Related issue/PR

Delete this section if there are no issues or pull requests that relate to this pull request.

  • Fixes #issue
  • Closes #PR

Checklist

  • Followed the guidelines in the CONTRIBUTING document
  • Added prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip] in the PR title if necessary
  • Tested and linted the code
  • Commented the code
  • Made corresponding changes to the documentation
  • Passed all pipeline checking

Checklist for maintainer

  • Use Squash and merge
  • Double-confirm the merge message has prefix [skip ci]/[ci skip]/[no ci]/[skip actions]/[actions skip]
  • Delete the branch after merge

@hiyosi hiyosi changed the title zIntroduce the simpleBuffer as implementation of the httputil.BufferPool. Introduce the simpleBuffer as implementation of the httputil.BufferPool. Sep 10, 2024
@f110
Copy link

f110 commented Sep 10, 2024

👍 LGTM

* Add warning log if rsa key

Signed-off-by: taniwa <[email protected]>

* fix wan log

Signed-off-by: taniwa <[email protected]>

---------

Signed-off-by: taniwa <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants