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

🩹[Fix]: Added respects body immutability to ctx.Body() and ctx.BodyRaw() functions. #2812

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

asyslinux
Copy link
Contributor

@asyslinux asyslinux commented Jan 26, 2024

Description

Added respects body immutability to ctx.Body() and ctx.BodyRaw() functions.

Related to #2757

Changes Introduced

  • Benchmarks: go test -run Test_Ctx_Body -bench=Benchmark_Ctx_Body -benchmem
  • Documentation Update: documentation already has full information and comments about body immutability.

Type of Change

  • Enhancement (improvement to existing features and functionality)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation: documentation already has full information and comments about body immutability.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

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

Can't we add a new methods like copyString like we did it getString or getBytes?

@asyslinux
Copy link
Contributor Author

asyslinux commented Jan 26, 2024

Can't we add a new methods like copyString like we did it getString or getBytes?

@efectn, Please read a issue: #2757 (comment)
We with @ReneWerner87 choose to use simple if, because body sometimes can be large and use func []byte []byte is not optimal in this case, simple if is cheaper. I checked via GodBolt, and compiler not inline this function by default, then call to function is more expensive then simple if, and also state of variable c.app.config.Immutable is unknown within compile-time.

Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

LGTM

ctx_test.go Outdated Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit bbfe9ac into gofiber:main Feb 2, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: Very rare panics in ctx.BodyParser or in fasthttp under unknown conditions
3 participants