gin icon indicating copy to clipboard operation
gin copied to clipboard

fix(context): make set and get header thread safe

Open saksham-arya-z opened this issue 1 year ago • 10 comments

fixes #4100

saksham-arya-z avatar Nov 20 '24 10:11 saksham-arya-z

I do not recommend the gin framework to add concurrency safety for headers.

It should be up to the user to decide whether to add locks. The main reasons are:

  • This would increase the complexity of gin;
  • From the perspective of http.Request, there is no control over adding locks;
  • Combined with what is submitted in the current PR, the role of gin's mu is too multifaceted, and any operation could lead to a decrease in gin's performance;
  • In some performance-sensitive scenarios, adding locks would limit the application's capabilities.

flc1125 avatar Nov 22 '24 05:11 flc1125

@flc1125 Since concurrent read writes to the headers map will lead to application crash, I believe this protection should be present in the library itself.

Combined with what is submitted in the current PR, the role of gin's mu is too multifaceted, and any operation could lead to a decrease in gin's performance;

Yes, this could be an issue so have created a separate mutex for header map

A similar change has been made to get and set context: https://github.com/gin-gonic/gin/issues/700

saksham-arya-z avatar Nov 22 '24 07:11 saksham-arya-z

@saksham-arya-z

Thank you very much for accepting the feedback I provided.

However, I still insist that gin does not need to address the header competition issue.

For this, I sincerely apologize.

flc1125 avatar Nov 22 '24 08:11 flc1125

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 98.69%. Comparing base (3dc1cd6) to head (3a07160). :warning: Report is 224 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4101      +/-   ##
==========================================
- Coverage   99.21%   98.69%   -0.53%     
==========================================
  Files          42       44       +2     
  Lines        3182     2980     -202     
==========================================
- Hits         3157     2941     -216     
- Misses         17       26       +9     
- Partials        8       13       +5     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.68% <100.00%> (?)
-tags go_json 98.61% <100.00%> (?)
-tags nomsgpack 98.67% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.24 98.69% <100.00%> (?)
go-1.25 98.69% <100.00%> (?)
macos-latest 98.69% <100.00%> (-0.53%) :arrow_down:
ubuntu-latest 98.69% <100.00%> (-0.53%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Nov 25 '24 13:11 codecov[bot]

@saksham-arya-z Please resolve the conflicts.

appleboy avatar Jun 26 '25 01:06 appleboy

@appleboy done

saksham-arya-z avatar Jun 26 '25 11:06 saksham-arya-z

@saksham-arya-z Please update the master branch.

appleboy avatar Jun 30 '25 09:06 appleboy

@appleboy branch is already up to date

saksham-arya-z avatar Jun 30 '25 09:06 saksham-arya-z

@appleboy Are we merging this?

saksham-arya-z avatar Dec 04 '25 12:12 saksham-arya-z

I agree with @flc1125 points.

  • Combined with what is submitted in the current PR, the role of gin's mu is too multifaceted, and any operation could lead to a decrease in gin's performance;
  • In some performance-sensitive scenarios, adding locks would limit the application's capabilities.

appleboy avatar Dec 05 '25 03:12 appleboy