RE: https://github.com/gorilla/mux/issues/182

OK, so this happens because r.WithContext() in Go 1.7, which gorilla/mux uses in Go 1.7, creates a shallow copy of the request: https://github.com/golang/go/blob/master/src/net/http/request.go#L288-L298

This shallow copy, as you'd expect, changes the address of the request in gorilla/context's map[*http.Request]map..., effectively islanding any context values.

This is a shortcoming in gorilla/context: any library, mux included, will break it come Go 1.7 and the http.Request.WithContext() method.`

I'm not sure if there's a clean way to fix this: one option would be to call context.GetAll to copy all values from the old request before we call WithContext, and then save them back into the (shallow) copy, but if you have private types as keys (which is recommended), then we can't reliably achieve that. It would also add context back as a dependency to mux.

• [x] Add a warning to the README
• [ ] Document public API so that godoc users also notice the warning when using getters/setters.

cc/ @kisielk

Fix #2

Reasoning

RWMutex shows significant benefits in contentious environments. There is a small performance hit incurred in low-contention, single processor environments. However, the typical use case will be multi-processor, high contention, so this change seems justified.

Benchmarks

A benchmark function was written to test parallel getting and setting across many goroutines. Please see context_test.go:BenchmarkMutex for the full code.

The following benchmarks are the results when using the original Mutex, the new RWMutex and removing the defer calls:

Mutex:

BenchmarkMutex           200       9787629 ns/op
BenchmarkMutex-2         100      16161809 ns/op
BenchmarkMutex-8         100      21128937 ns/op
BenchmarkMutex-16        100      22455500 ns/op
BenchmarkMutex-32        100      22477323 ns/op

RWMutex:

BenchmarkMutex           200       9990271 ns/op
BenchmarkMutex-2         500       6410595 ns/op
BenchmarkMutex-8         500       3928236 ns/op
BenchmarkMutex-16        500       3946676 ns/op
BenchmarkMutex-32        500       3963064 ns/op

RWMutex, no defer:

BenchmarkMutex           500       5535964 ns/op
BenchmarkMutex-2         500       4876897 ns/op
BenchmarkMutex-8         500       3712937 ns/op
BenchmarkMutex-16        500       3633307 ns/op
BenchmarkMutex-32        500       3661723 ns/op

Please review this pull request and let me know if the benchmark needs to be changed, or additional benchmarks need to be added.

Thank you.

I have added a single method GetAll(r *http.Request) map[interface{}]interface{} that returns the full context of an request, when exists; otherwise, an empty map. This feature is useful if you want to get the full context of an request.

Request aren’t pooled, so we can wrap the request Body and store attributes there. This means that each request has its own place to store attributes, without having to go through a mutex. It also means that Clear can be dropped in a later release, since the Request will be eligible for GC and should do our job for us.

Before:

> go test -bench .
PASS
BenchmarkMutexSameReadWrite1      100000         13965 ns/op
BenchmarkMutexSameReadWrite2       50000         28821 ns/op
BenchmarkMutexSameReadWrite4       30000         57187 ns/op
BenchmarkMutex1    20000         86459 ns/op
BenchmarkMutex2    10000        222826 ns/op
BenchmarkMutex3    20000         90147 ns/op
BenchmarkMutex4      200       7688993 ns/op
BenchmarkMutex5       30      50292186 ns/op
BenchmarkMutex6        5     298536790 ns/op
ok      _/Users/jabley/git/context  19.730s

With the mutex removed:

> go test -bench .
PASS
BenchmarkMutexSameReadWrite1      100000         12700 ns/op
BenchmarkMutexSameReadWrite2       50000         25692 ns/op
BenchmarkMutexSameReadWrite4       30000         50888 ns/op
BenchmarkMutex1    20000         71686 ns/op
BenchmarkMutex2    10000        197388 ns/op
BenchmarkMutex3    20000         77450 ns/op
BenchmarkMutex4      200       6996854 ns/op
BenchmarkMutex5       30      42156099 ns/op
BenchmarkMutex6        5     261614479 ns/op
ok      _/Users/jabley/git/context  17.295s

Since it is an API breaking change, I do not expect you to merge it as-is so it is more FYI.

The rationale is that ints are generally not useful as keys in the case of a context that is shared across multiple unrelated packages. The different packages must use a cooperative way of assigning globally unique keys.

Most projects will use key1 = 0, key = 1, ... but using an int via an interface{} adds an unneeded pointer indirection to the key. Using string as keys results in unneeded string comparison for map lookups (or worse, hashing!) which is much less efficient than a simple embedded uintptr comparison.

The trade off is process globally unique key, faster lookup and denser map by not using 'const' keys.

Note that this is an incompatible change, it changes the signature of the functions.

I'd like to duplicate this package using a new name: "registry". Because that's what a context really is.

The context package can be kept available for who is using it, but we favor the new name internally and in the site links and docs.

Worthless? :)

Not sure how to fix this, but I keep getting this error on Windows when I try to get any package using context

D:\Développement\golang\src\github.com>go install github.com/gorilla/pat
# github.com/gorilla/mux
D:\Developpement\golang\src\github.com\gorilla\mux\mux.go:12: can't find import: "github.com/gorilla/context"

Any idea how I can prevent this?

https://github.com/gorilla/context/blob/master/context.go#L31 ,

What is the need for read lock in GET() , how will simultaneous read effect the variables ?? In write, the lock makes sense , otherwise there is a chance for a goroutine to see a inconsistent view .. What i want to understand why is the lock implemented in Get() ?

Peeking at the code, I noticed the Get() locked the map too, while in fact concurrent reads are safe. Shouldn't you use a RWMutex instead (http://golang.org/pkg/sync/#RWMutex)?

Thanks for the great toolkit, Martin

I notice that there is only one mutex in the context package. I assume if some request call context.Set and therefore hold the write lock, at that moment, any other request will have to wait for it while they don't need to, since the context is request-oriented and preventing race conditions between requests is not its concern. Would it be nice if we let each request have its own mutex.

var (
    mutexes = make(map[http.Request]sync.RWMutex)
    data  = make(map[*http.Request]map[interface{}]interface{})
    datat = make(map[*http.Request]int64)
)

I currently have a helper that is used to render to a http.ResponseWriter. Currently, the renderer is built conditionally on parameters in the request in a middleware far away from the request handling code. The result is that I could have relatively clean handler code like this:

func SearchHandler (w http.ResponseWriter, req *http.Request) {
  someResult := <do some stuff that doesn't ever fail>
  renderJSON(w, http.StatusOK, someResult)
}

However, if I want to access the context created by the middleware, I also need to have req passed to renderJSON just to get the context.

If instead, I could key the context based on the http.ResponseWriter, I would be able to keep my signatures cleaner.

Perhaps there are other solutions here too that involve completely different patterns than I'm currently using.