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(context): make set and get header thread safe #4101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

saksham-arya-z
Copy link

@saksham-arya-z saksham-arya-z commented Nov 20, 2024

fixes #4100

Copy link

@vedansh-zomato vedansh-zomato left a comment

Choose a reason for hiding this comment

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

LGTM

@saksham-arya-z saksham-arya-z changed the title Make set and get header thread safe fix(context): make set and get header thread safe Nov 20, 2024
@flc1125
Copy link
Contributor

flc1125 commented Nov 22, 2024

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.

context.go Outdated
@@ -992,6 +995,8 @@ func (c *Context) Header(key, value string) {

// GetHeader returns value from request headers.
func (c *Context) GetHeader(key string) string {
c.mu.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

mu bundles too many functions, even if there are no header-related operations. But as long as mu is used, locking will also occur in the logic related to headers. This will degrade performance.

Copy link
Author

Choose a reason for hiding this comment

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

created a separate mutex for this

add reader lock
@saksham-arya-z
Copy link
Author

@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: #700

fix lint
@flc1125
Copy link
Contributor

flc1125 commented Nov 22, 2024

@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.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.99%. Comparing base (3dc1cd6) to head (cd86308).
Report is 83 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4101      +/-   ##
==========================================
- Coverage   99.21%   98.99%   -0.22%     
==========================================
  Files          42       44       +2     
  Lines        3182     3397     +215     
==========================================
+ Hits         3157     3363     +206     
- Misses         17       24       +7     
- Partials        8       10       +2     
Flag Coverage Δ
?
-tags "sonic avx" 98.99% <100.00%> (?)
-tags go_json 98.99% <100.00%> (?)
-tags nomsgpack 98.98% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 98.99% <100.00%> (-0.22%) ⬇️
go-1.22 98.99% <100.00%> (?)
macos-latest 98.99% <100.00%> (-0.22%) ⬇️
ubuntu-latest 98.99% <100.00%> (-0.22%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

ctx.Header is not thread safe
5 participants