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 keys get set #702

Closed
wants to merge 7 commits into from
Closed

Fix context keys get set #702

wants to merge 7 commits into from

Conversation

rohan-j-shah
Copy link

Fix for : fatal error: concurrent map read and map write

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage increased (+0.01%) to 94.099% when pulling 5a6b31c on goibibo:fix_context_keys_get_set into f931d1e on gin-gonic:develop.

context.go Outdated
@@ -160,14 +162,18 @@ func (c *Context) Set(key string, value interface{}) {
if c.Keys == nil {
c.Keys = make(map[string]interface{})
}
c.KeysLocker.Lock()

Choose a reason for hiding this comment

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

I would suggest taking it before nil check to avoid a set being lost..

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.099% when pulling 9ab71be on goibibo:fix_context_keys_get_set into f931d1e on gin-gonic:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.099% when pulling 9ab71be on goibibo:fix_context_keys_get_set into f931d1e on gin-gonic:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 94.099% when pulling 9ab71be on goibibo:fix_context_keys_get_set into f931d1e on gin-gonic:develop.

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage increased (+0.01%) to 94.099% when pulling 8c413be on goibibo:fix_context_keys_get_set into f931d1e on gin-gonic:develop.

Copy link
Contributor

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

Please rebase correctly, than you can get some proper review.

@maxatome
Copy link
Contributor

The locking mechanism involved in this pull request is not needed as the original gin.Context should not be used outside the original gin gonic goroutine without copying it (Copy method). In fact, it even hides some biggest errors!
If you have a "concurrent map read and map write" error, it means that you did a mistake. Probably using the same context in different goroutines without copying it.
If you need to share a gin.Context between several goroutines of your own, you first have to copy (Copy method) it then handle the locking mechanism yourself at your level, perhaps by composing the gin.Context in your own struct.
The problem with your patch is that it hides completely the fact that gin.Context instances are reused once the request is handled at the gin gonic level.

@tsirolnik
Copy link
Contributor

@maxatome I'm not sharing the context but I still do get panics sometimes even though the key was set.

See this issue #836

@thinkerou
Copy link
Member

@rohan-j-shah please re-commit the pull request to master branch, thanks!

@thinkerou
Copy link
Member

closing, ref #1391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants