Skip to content

context.Set() doesn't lock the map for writing #273

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

Closed
polds opened this issue Nov 25, 2015 · 8 comments
Closed

context.Set() doesn't lock the map for writing #273

polds opened this issue Nov 25, 2015 · 8 comments
Assignees

Comments

@polds
Copy link

polds commented Nov 25, 2015

So I'm sure this was done on purpose for speed issues but recently ran into an issue where we had a middleware issuing unique Request IDs and saving them using context.Set(). Under high concurrency we found that requests began to start sharing request ids and found it was caused by the underlying map for the store not locking.

@vishr
Copy link
Member

vishr commented Nov 25, 2015

Each request runs in a separate goroutine so unless you introduce goroutines within, there shouldn't be any problem. Further the underlying map get reset for every new request.

@vishr vishr self-assigned this Nov 25, 2015
@polds
Copy link
Author

polds commented Nov 30, 2015

That must have been my problem then. We were introducing additional goroutines within a request. What was happening was the goroutine for Request 1 was receiving the request ID assigned by the middleware of Request 2.

@vishr
Copy link
Member

vishr commented Nov 30, 2015

In that case, you must acquire a lock while accessing Context#Set and Context#Get APIs.

@polds
Copy link
Author

polds commented Nov 30, 2015

With the storage being unexported and no mutex added to the struct how would you recommend to obtain the lock without creating a copy of the Context?

@vishr
Copy link
Member

vishr commented Nov 30, 2015

For an incoming request, a sync.Mutex should be created and used to access the storage in Context. This lock could be part of Context itself, stored in golang.org/x/net/context. For reference: http://labstack.com/echo/guide/request

Update: You can also store your own synchronized map in net/Context.

@polds
Copy link
Author

polds commented Dec 2, 2015

Using the withValue seems to work. I'm wondering now what the difference in using the store in the echo.Context vs storing things in the net.Context?

example:

What's the difference between:

c.Set("key", "val")
c.Get("key").(string)

and

c.Context = context.WithValue(nil, "key", "val")
c.Value("key").(string)

@vishr
Copy link
Member

vishr commented Dec 2, 2015

net/Context was added recently. The only difference now is that internal storage for echo.Context is a map, so you can store multiple key/val pair but with net/Context, by default its just one key val, although val can be a map again 😄. There is another solution as I mentioned earlier, have a synchronized map and store it in net.Context.

@polds
Copy link
Author

polds commented Dec 2, 2015

Appreciate the responses. Closing this.

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

No branches or pull requests

2 participants