Closed asyslinux closed 7 months ago
welcome[bot]
commented
9 months ago Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord
asyslinux
commented
9 months ago Additionally, maybe there is no bug, can my additional context with Timeout affect when go func() and the second call to ctx.Body -> fasthttp.Request is executed after my context is canceled by timeout, and fiber.Handler has already returned - fiber.Handler { return func(ctx *fiber.Ctx) error {
?
I can remove my extra context with timeout and go func() too, because I don't really need it anymore.
sancar
commented
9 months ago I recently came across a similar problem. It seems that Immutable option is not used for ctx.Body() even tough the documentation says so.
ReneWerner87
commented
9 months ago can confirm it, no copy is made with the body method, even if immutable is true, if necessary we could correct this
another comment is that in a concurrent function, after the end of the run (timeout), an attempt is made to fetch the content of body is also critical we can't fix this, fasthttp is so fast because the request and response capsules are reused in the next run (no matter if immutable is on or not) if an attempt is made to take something from the response after the timeout, even though it is already integrated into another process, this can certainly lead to problems a copy would have to be created shortly beforehand
asyslinux
commented
9 months ago can confirm it, no copy is made with the body method, even if immutable is true, if necessary we could correct this
It would be great if there was an additional option in Fiber or a fix.
asyslinux
commented
9 months ago The problem in panic when &c.fasthttp.Request is nil pointer.
What should be done to prevent the application from panic? Since fasthttp clearly states that there is no recover in fasthttp itself:
// Take into account that no `panic` recovery is done by `fasthttp` (thus any `panic` will take down the entire server).
// Instead the user should use `recover` to handle these situationsI already configured my app to use app.Use(recover.New()), but when &c.fasthttp.Requestdisappeared, in this case my application does not recovering.
Below I will write what can be improved in fiber by adding a new option. But nevertheless, even if you start working with the body at the very beginning, some kind of edge case may still arise and &c.fasthttp.Request will be nil pointer, even at the very beginning. Of course, in paragraph 3 below I can suggest improvements in fiber.
It’s not entirely clear how to make it not crash in a panic so that recover works, because app.Use(recover.New()) doesn’t help me.
About PR with immutability for body:
ctx.Body() - does not respect the immutable option in relation to the body.
ctx.BodyRaw() - does not respect the immutable option in relation to the body.
ctx.BodyParser(&mybody) - respects the immutable option in relation to the body, since all data there enters the structure through c.app.getString(), which in turn will be a function depending on the Immutbale option.
And it is clear that there is another option ctx.Request().Body() - you can get the body directly from &c.fasthttp.Request and this can not be immutable, because this is direct access to fasthttp body.
myimmutablepostbody := utils.CopyBytes(ctx.Body())
myimmutablepostbody := utils.CopyBytes(ctx.BodyRaw())
myimmutablepostbody := utils.CopyBytes(ctx.Request().Body())And then work with the already copied body. Also can work with copied body in additional context with custom go func().
ctx.Body() and ctx.BodyRaw() respects immutability (but only through additional option "StrictImmutableBody"), then I or any another man, can make PR, like this PR is be acceptable in this manner (add c.app.getBody() like c.app.getString())?:https://go.dev/play/p/NBFC_NLT3lV
And also will do modify ctx.Body() and ctx.BodyRaw() methods for using new function:
c.app.getBody(c.fasthttp.Request.Body())
Thanks.
ReneWerner87
commented
9 months ago @asyslinux thanks for the analysis
exactly,ctx.Body() and ctx.BodyRaw() should respect the Immutable setting and make a copy with utils.CopyBytes
an additional setting is not necessary, only the query of the setting in the respective methods and the making of the copy at the end shortly before the return
ReneWerner87
commented
9 months ago @asyslinux could you support us with a PR ?
asyslinux
commented
9 months ago @asyslinux could you support us with a PR ?
@ReneWerner87, yes, I will make a PR on next week, with addons to documentation(wiki) and I need a little time to test edge cases when a nil pointer error occurs in fasthttp, since for some reason app.Use(recover.New()) does not help me, even with the ctx.BodyParser() function. Since I have a high load application, I can now reproduce and test. Then I will write a full explain in the documentation, how correctly use body with fiber and fasthttp. Also I will add example of mutex-free parallel high-throughput app for POST requests processing, may be for some people this example can help write high efficient POST request processing apps.
asyslinux
commented
9 months ago and the making of the copy at the end shortly before the return
@ReneWerner87 I don't quite understand which code option is preferable:
ctx.Body() and ctx.BodyRaw() and the make a copy, if Immutable: truec.app.getBody() function []byte []byte like in this example (https://go.dev/play/p/NBFC_NLT3lV) and use this function like c.app.getBody(c.fasthttp.Request.Body()), same another functions c.app.getString() / c.app.getBytes()Which method is better in the case of the body?
ReneWerner87
commented
9 months ago I prefer 1. @gaby @efectn what do you think?
asyslinux
commented
9 months ago @ReneWerner87, I checked what will happen in both cases, firstly, Go by default does not inline the getBody() function from the 2nd case inside the App. Additionally, the state of the immutable variable at compile time is unknown. Therefore, in case 2 there will always be a function call, and it is more expensive than simple "if".
Even such a simple example with the getBody() function is not inlined by default, but here the state of the variable is known in advance at the compilation stage. https://godbolt.org/z/ddjGMnW6x
I prefer 1 too. Let's wait what others say.
ReneWerner87
commented
8 months ago @asyslinux you are welcome to start, further comments may be made in the review, otherwise it looks ok for solution 1
asyslinux
commented
8 months ago @ReneWerner87, please review PR when you have a time. I will add a little later the results of practice tests that cause panic(I'm still dealing with this problem) and an example of a high-performance application for receiving and processing a large number of POST requests.
Bug Description
Immutable: true
Application receives 1000rps+ requests with POST data. In 6 months of continuous operation of the application, I encountered these two panics only 2 times. I do not understand, how can be nil pointers with immutable true...I don't even know what to guess.
It seems to me that this can only happen if some kind of premature interruption of a request in fasthttp and there comes a moment when my code in the application is already running and was not interrupted for some reason and in a rare case a call to the nil pointer occurs. But this is just a guess.
Fiber settings:
This panics from 1 server:
Requests with 499 HTTP CODE (client closed request) from nginx:
This panics from 2 server:
Requests with 499 HTTP CODE (client closed request) from nginx:
How to Reproduce
I do not know how to reproduce, I guess this sometimes happens with 499 (client closed request). Maybe I missed something when writing an application to process POST requests. Maybe need to write code using OnClose? In case the client closes the connection ahead of time. But just how to do this, I don’t know at what point in time the client will break the connection, in my code anywhere there can be access to headers, body, request parameters, this is not an atomic operation.
v2@v2.50.0/ctx.go:376 https://github.com/gofiber/fiber/blob/af3999835f33943c7d63aa1cfc70e64e94319eca/ctx.go#L376 v2@v2.50.0/ctx.go:382 https://github.com/gofiber/fiber/blob/af3999835f33943c7d63aa1cfc70e64e94319eca/ctx.go#L382 v2@v2.50.0/ctx.go:560 https://github.com/gofiber/fiber/blob/af3999835f33943c7d63aa1cfc70e64e94319eca/ctx.go#L560
Expected Behavior
Usually everything works without problems, serves a lot of requests, never panics for months.
Below in my code you can see that panic occurs not only when the body is processed for the first time, but also when it is processed again, also on line 1040 of my code. That is, imagine that there was a body in fasthttp and there was no panic on line 1033, and then it disappeared somewhere in fasthttp and a panic happened on line 1040. Yes, and the first processing of a POST body request can also cause panic right away.
More precisely, the entire fasthttp.Request disappears somewhere or does not have time to be copied when client closing connection, but my code is executed and essentially accesses fasthttp.Request via fiber. But interesting case, when on line 1033 fasthttp.Request have data, but little later on line 1040 fasthttp.Request is no have data, how data on request with immutability enabled, which should have already been copied and successufully accessed on line 1033, suddenly disappeared somewhere and on line 1040 i get a nil pointer.
Fiber Version
v2.50.0 and below
Code Snippet (optional)
When processing POST requests, I made an additional context with a timeout and placed the request processing in the go func(), this was done just in case, so that if anything happens, the processing of the POST request is guaranteed to complete. But I can definitely say that if panic occurs, the timeout of 3 seconds does not occur.
Partly my code:
Checklist: