search

valyala / fasthttp

Fast HTTP package for Go. Tuned for high performance. Zero memory allocations in hot paths. Up to 10x faster than net/http
MIT License
21.91k stars 1.76k forks source link

NewFastHTTPHandler may cause improper memory reuse #1859

Closed sigmundxia closed 2 months ago

sigmundxia commented 2 months ago

Hi developers,

I encountered a strange problem when using the middleware of fiber, a downstream software that depends on fasthttp. I think this problem may be related to fasthttp.

To better illustrate this problem, I added a test for fasthttp to simulate the process of fiber using middleware. As a result, this test failed and reported an error in line 177:

https://github.com/sigmundxia/fasthttp/blob/a165213f6370f59e5d6414f4f573eb6519c472ca/fasthttpadaptor/adaptor_test.go#L142-L187

As I mentioned in the code comments, this test can pass as long as line 172 is commented out, but line 172 is not related to cookies, which indicates that fasthttp may have experienced improper memory reuse in it.

I think there are two possible directions of the solution:

  1. Fix the improper memory reuse problem in fasthttp. I have basically located the code that causes this problem. If you are sure that this is indeed a bug, I am happy to submit a PR to fix this problem.

  2. Confirm that the way to use middleware in fiber is inappropriate. Since the fiber implementation ignores ResponseWriter and r in the middleware and restarts from ctx, this may be inappropriate. If you are sure that this is the reason for the error, I will try to communicate with the fiber developers.

Thank you for your attention! If you have any questions or would like to discuss further with me, please feel free to contact me.

Best wishes.

newacorn commented 2 months ago

The issue is related to fasthttp, where shared memory is used when copying request cookies from fasthttp to net/http. Your pull request is welcome.

sigmundxia commented 2 months ago

The issue is related to fasthttp, where shared memory is used when copying request cookies from fasthttp to net/http. Your pull request is welcome.

Thanks! I've submitted a pull request that will hopefully fix this.

sigmundxia commented 2 months ago

Since the pull request has been merged, I think this issue can be closed :)