Skip to content
This repository has been archived by the owner. It is now read-only.

Fix racy code in Get #14

Merged
merged 1 commit into from Jun 4, 2014
Merged

Fix racy code in Get #14

merged 1 commit into from Jun 4, 2014

Conversation

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Jun 4, 2014

I was looking for races in docker and found this :)

context.go Outdated
@@ -31,8 +31,9 @@ func Set(r *http.Request, key, val interface{}) {
func Get(r *http.Request, key interface{}) interface{} {
mutex.RLock()
if data[r] != nil {

This comment has been minimized.

@kisielk

kisielk Jun 4, 2014
Member

Good catch 👍 While you're here, can you change this to:

if ctx := data[r]; ctx != nil {
    value := ctx[key]
...

to avoid having to look up the key twice.

This comment has been minimized.

@LK4D4

LK4D4 Jun 4, 2014
Author Contributor

Done!

kisielk added a commit that referenced this pull request Jun 4, 2014
Fix racy code in Get
@kisielk kisielk merged commit 14f550f into gorilla:master Jun 4, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@kisielk
Copy link
Member

@kisielk kisielk commented Jun 4, 2014

Thanks 👍

@LK4D4 LK4D4 deleted the LK4D4:fix_race_in_get branch Jun 4, 2014
@mperham

This comment has been minimized.

Copy link

@mperham mperham commented on 9af5636 Jul 29, 2014

Out of curiousity, why not use defer so you know the mutex will be unlocked only once the method is finished? This bug would not happen with defer.

This comment has been minimized.

Copy link
Contributor Author

@LK4D4 LK4D4 replied Jul 29, 2014

@mperham defer has huge overhead for such small functions.
http://lk4d4.darth.io/posts/defer/

This comment has been minimized.

Copy link

@mperham mperham replied Jul 29, 2014

@LK4D4 Wow, TIL. Thanks!

This comment has been minimized.

Copy link
Member

@kisielk kisielk replied Jul 29, 2014

Yeah actually it was introduced as a result of a PR to not use defer :/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.