Add concurrency safe context option. #1158

andreiavrammsd · 2018-07-04T18:57:31Z

Update: Safe context is used by default, with no option to disable it.

Update: Safe context is by default, with the option to use it unsafe.

If SafeContext option set to true on an Echo server instance, contexts will be concurrency safe.

I met the #273 issue while running GraphQL with concurrent resolvers, and I think it would be nice to offer the option of concurrency safe contexts.

Usage

e := echo.New()
e.SafeContext = true

Benchmarks

Non safe:

goos: linux
goarch: amd64
pkg: github.com/andreiavrammsd/echo
5000000	       408 ns/op
PASS

func BenchmarkContext_Store(b *testing.B) {
	for i := 0; i <= b.N; i++ {
		e := &Echo{}

		c := &context{
			echo: e,
		}
		c.Set("name", "Jon Snow")
		c.Get("name")
	}
}

Safe:

goos: linux
goarch: amd64
pkg: github.com/andreiavrammsd/echo
3000000	       545 ns/op
PASS

func BenchmarkContext_SafeStore(b *testing.B) {
	for i := 0; i <= b.N; i++ {
		e := &Echo{}
		e.SafeContext = true

		c := &context{
			echo: e,
		}
		c.Set("name", "Jon Safe")
		c.Get("name")
	}
}

codecov · 2018-07-04T19:01:17Z

Codecov Report

Merging #1158 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
+ Coverage   81.78%   81.82%   +0.03%     
==========================================
  Files          26       26              
  Lines        1933     1937       +4     
==========================================
+ Hits         1581     1585       +4     
  Misses        243      243              
  Partials      109      109

Impacted Files	Coverage Δ
context.go	`74.68% <100%> (+0.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8896575...a91ca8c. Read the comment docs.

stale · 2018-11-30T13:52:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

andreiavrammsd · 2018-11-30T13:57:02Z

Is this explicitly not desired or has it been missed? I can close it with no problem if it's not in the Echo vision.

alexaandru · 2019-02-02T10:47:35Z

Most likely missed @andreiavrammsd Thank you for the PR.

Looks like a good idea to me. In fact I don't see why anyone would prefer the unsafe version. Thoughts @vishr @im-kulikov ? In fact, I would propose an "opposite" PR: change Echo so that safe is the default option and add an UnsafeContext option instead (for cases where people both: 1) know exactly what they are doing and 2) they really need every bit of speed they can squeeze out of Echo).

im-kulikov

Wow. It's interesting, but I'm leaning towards @alexaandru. This way, we won't break backward compatibility and give additional features to those who really need it.

andreiavrammsd · 2019-02-02T12:46:43Z

@im-kulikov, wouldn't it break compatibility if safe context were to be default because it would be a sudden change of default behaviour?

im-kulikov · 2019-02-02T12:49:03Z

hmmm.. make sense...

im-kulikov · 2019-02-02T12:53:27Z

I think there's been a misunderstanding. It will be more predictable if the context is to be thread safe out of the box.

alexaandru · 2019-02-02T15:58:40Z

If anyone depends on their application crashing, then there's a very easy fix for that: just use math/rand and from time to time call panic(). That should bring things back to as they were before this :-)

Now joking aside, I don't see how anyone could raise the "backward compatibility" card, when all we're doing is touching the internals and eliminate a possible crash condition. No signature changes, we don't remove anything from the public interface, we simply ensure that under certain conditions, the app will not crash.

Now OTOH, not sure that having all requests both write and read from context is by itself a good pattern. If you need all your requests to do that concurrently, probably you're missing a datastore of some sort (could be in memory, Redis, BoltDB, whatever). I mean locks only ensure it is safe to do so, but it doesn't make it right :-) Which is probably one of the reasons why no one complained about it so far...

I still think we should fix it, I just think we should not encourage using context like it's some sort of database.

andreiavrammsd · 2019-02-03T13:51:09Z

One could argue that locking is degrading performance, but if you get to this situation, you'll be able to use the UnsafeContext.

I'll update the PR with the opposite implementation by adding the UnsafeContext option.

alexaandru · 2019-02-03T19:18:42Z

Yeah one could try to argue that, but it would be a weak argument. There's no framework that I know of that has the speed as part of their public interface. Nowhere in the README (or any other docs) does Echo say "a request should complete within X ms". I think we're safe :-)

Thank you for updating the PR!

andreiavrammsd · 2019-02-08T20:09:43Z

@alexaandru, @im-kulikov, please take a look.

alexaandru · 2019-02-09T18:40:49Z

@andreiavrammsd could you please squash the branch? Makes it easier to review/comment on it. Thanks!

andreiavrammsd · 2019-02-09T20:04:37Z

Sure, it's done.

alexaandru · 2019-02-09T20:24:39Z

Thank you @andreiavrammsd :) Looks good to me! @im-kulikov @vishr ?

im-kulikov

🎉 Great job! Thanks. SGTM

vishr · 2019-02-09T22:16:57Z

LGTM, ok to merge. I updated this issue labstack/echox#37. We need to find time to update the docs.

vishr · 2019-02-10T00:45:34Z

@alexaandru
As this is still not merged, I have a recommendation: why don't we use sync.Map for store in Context?

andreiavrammsd · 2019-02-10T07:04:38Z

sync.Map was not really designed for general purpose, as the docs state.

Also, benchmarks using a regular map indicate zero allocations, while a sync.Map used 2. Perphaps nobody dies from 2 allocations, but I think you should stay away from adding a few allocations here, maybe some others with a new feature one day and so on.

Mutex: BenchmarkContext_Store-4 100000000 117 ns/op 0 B/op 0 allocs/op
sync.Map: BenchmarkContext_Store-4 100000000 131 ns/op 32 B/op 2 allocs/op

Both executed with: GOMAXPROCS=4 go test -bench=Context_ -benchtime=10s -benchmem

I believe a simple map is safer for any use case a user may have.

The code with sync.Map (besides the below, some adjustments would be needed for creating a new context and reseting one):

context struct {
	request  *http.Request
	response *Response
	path     string
	pnames   []string
	pvalues  []string
	query    url.Values
	handler  HandlerFunc
	store    sync.Map
	echo     *Echo
}

func (c *context) Get(key string) interface{} {
	v, _ := c.store.Load(key)
	return v
}

func (c *context) Set(key string, val interface{}) {
	c.store.Store(key, val)
}

And if you still want to offer the UnsafeContext option for maximum performance, then we need a simple map, too.

Benchmarks environment:

go version

go version go1.11.5 linux/amd64

go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/msd/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/msd/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/msd/work/github.com/andreiavrammsd/echo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build633382178=/tmp/go-build -gno-record-gcc-switches"

vishr · 2019-02-11T20:52:21Z

sync.Map was not really designed for general purpose, as the docs state.

Also, benchmarks using a regular map indicate zero allocations, while a sync.Map used 2. Perphaps nobody dies from 2 allocations, but I think you should stay away from adding a few allocations here, maybe some others with a new feature one day and so on.

Mutex: BenchmarkContext_Store-4 100000000 117 ns/op 0 B/op 0 allocs/op
sync.Map: BenchmarkContext_Store-4 100000000 131 ns/op 32 B/op 2 allocs/op

Both executed with: GOMAXPROCS=4 go test -bench=Context_ -benchtime=10s -benchmem

I believe a simple map is safer for any use case a user may have.

The code with sync.Map (besides the below, some adjustments would be needed for creating a new context and reseting one):
context struct {
	request  *http.Request
	response *Response
	path     string
	pnames   []string
	pvalues  []string
	query    url.Values
	handler  HandlerFunc
	store    sync.Map
	echo     *Echo
}

func (c *context) Get(key string) interface{} {
	v, _ := c.store.Load(key)
	return v
}

func (c *context) Set(key string, val interface{}) {
	c.store.Store(key, val)
}
And if you still want to offer the UnsafeContext option for maximum performance, then we need a simple map, too.

Benchmarks environment:

go version
go version go1.11.5 linux/amd64
go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/msd/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/msd/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/msd/work/github.com/andreiavrammsd/echo/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build633382178=/tmp/go-build -gno-record-gcc-switches"

Got it, I am a little reluctant to introduce a new option and would favor it as default. Can you run a benchmark test (already in the repo) to see if there is any different with/without lock or is it even in the path?

alexaandru · 2019-02-11T20:58:58Z

LGTM, ok to merge. I updated this issue labstack/echox#37. We need to find time to update the docs.

Yup, the docs need some love :-) I'll be happy to take a stab at that, but only a little later this month. First, I want to help close as many tickets and PRs as possible, after that I'll focus on the docs.

alexaandru · 2019-02-11T21:10:53Z

@vishr He did provide the benchmarks in the initial post: 545 ns/op with locks vs 408 ns/op without.
That being said, those benchmarks mean very little as they only test one single case out of a large variety of cases (the case where each request would do both a Set and a Get). We can have a lot of possible combinations (and thus a lot of possible change in performance): not using Set/Get at all (yay, not affected), using a lot of Set() or a lot of Get(), using it only on few resources, using it on many, etc. etc. and all their possible combinations. So it's basically impossible to say "the performance will decrease by X% after adding this"... it depends entirely on how the context is used by each end user in particular.

So let's see, what options we have:

the option in here, to default to safe (lock) and offer option for unsafe (if they want extra performance);
same as 1., but without the option to remove safety;
keep it as is (don't add lock) but make it clear in the docs that it is not thread safe AND show end users how to extend context to add safety around it (which is pretty trivial, now that I think of it...).

As I already said, I believe the case of heavy read/write from/to context would be rather the exception than the norm. The context is not a database... :-) The option no. 3 starts to sound appealing to me... :-)

vishr · 2019-02-11T21:14:46Z

He did provide the benchmarks in the initial post: 545 ns/op with locks vs 408 ns/op without.

I saw those and we should be ok with that difference. However, I was referring to routing benchmarks https://github.com/labstack/echo/blob/master/router_test.go#L935, if these are not affected - we should be good.

andreiavrammsd · 2019-02-11T21:33:11Z

Why would the router be affected? I don't see how the lock is attained in the router. Am I missing something?

Anyway, I ran the router benchmarks twice for each branch:

goos: linux
goarch: amd64
pkg: github.com/labstack/echo/v4

branch: optional-concurrency-safe-context

BenchmarkRouterStaticRoutes-4            1000000             15580 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGitHubAPI-4                500000             28866 ns/op               0 B/op          0 allocs/op
BenchmarkRouterParseAPI-4                3000000              4982 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGooglePlusAPI-4           2000000              8350 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/labstack/echo/v4     76.596s

BenchmarkRouterStaticRoutes-4            1000000             15992 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGitHubAPI-4                500000             28664 ns/op               0 B/op          0 allocs/op
BenchmarkRouterParseAPI-4                3000000              4992 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGooglePlusAPI-4           2000000              8465 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/labstack/echo/v4     77.293s


branch: master

BenchmarkRouterStaticRoutes-4            1000000             15708 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGitHubAPI-4                500000             28530 ns/op               0 B/op          0 allocs/op
BenchmarkRouterParseAPI-4                3000000              4951 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGooglePlusAPI-4           2000000              8273 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/labstack/echo/v4     76.203s

BenchmarkRouterStaticRoutes-4            1000000             15693 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGitHubAPI-4                500000             29020 ns/op               0 B/op          0 allocs/op
BenchmarkRouterParseAPI-4                3000000              4978 ns/op               0 B/op          0 allocs/op
BenchmarkRouterGooglePlusAPI-4           2000000              8269 ns/op               0 B/op          0 allocs/op
PASS
ok      github.com/labstack/echo/v4     76.534s

Indeed, if "it doesn't feel right" it can be a good idea to leave everything as it is, like @alexaandru said, and just clearly state the situation in the docs and give the custom context solution (it's actually what I did on the project where I met the issue).

vishr · 2019-02-11T21:48:08Z

Why would the router be affected? I don't see how the lock is attained in the router. Am I missing something?

Just wanted to double check it's been long time ;). I would propose to make Context/Get/Set synchronized using a lock, default.

alexaandru · 2019-02-11T22:10:01Z

@vishr that's exactly how the PR works right now (Get/Set protected by a lock, by default). Only if end users opt in to avoid locking, they can do so. Are you OK with keeping the new option UnsafeContext ?

If we keep it, there's a couple suggestions I have: 1) let's make it clear that it is solely for backward compatibility purposes and we do not recommend it (including in the commit message but also in the docs) and 2) possibly mark it as obsolete and "to be removed in the next major version" ? I mean, we should just be safe by default... For the few cases where this will impact their performance, maybe it's worth the trouble of adding it, but long term I don't think we should keep it.

Also, if you're suggesting to remove the unsafe option, I'm not opposed to that :-)

vishr · 2019-02-11T22:12:11Z

Are you OK with keeping the new option UnsafeContext ?

I don't want to introduce another option in the code. The overhead of a lock is negligible - let's keep it by default.

alexaandru · 2019-02-11T22:13:50Z

Works for me! :-)

andreiavrammsd · 2019-02-12T16:57:45Z

Please trigger the Travis build, it looks like it got stuck.

vishr · 2019-02-12T18:56:38Z

thanks for your contribution 🎉

andreiavrammsd · 2019-02-12T19:44:02Z

Thank you, too, for your time!

stale bot added the wontfix label Nov 30, 2018

stale bot removed the wontfix label Nov 30, 2018

im-kulikov reviewed Feb 2, 2019

View reviewed changes

andreiavrammsd force-pushed the optional-concurrency-safe-context branch from cb0d7de to 8c4fa85 Compare February 8, 2019 19:58

andreiavrammsd force-pushed the optional-concurrency-safe-context branch from 8c4fa85 to e70766a Compare February 9, 2019 19:59

im-kulikov approved these changes Feb 9, 2019

View reviewed changes

vishr mentioned this pull request Feb 9, 2019

Update doc labstack/echox#37

Open

vishr assigned alexaandru Feb 9, 2019

Use concurrency safe context by default.

a91ca8c

andreiavrammsd force-pushed the optional-concurrency-safe-context branch from e70766a to a91ca8c Compare February 12, 2019 16:32

vishr approved these changes Feb 12, 2019

View reviewed changes

vishr merged commit 3d73323 into labstack:master Feb 12, 2019

andreiavrammsd deleted the optional-concurrency-safe-context branch February 12, 2019 19:44

nikunjbadjatya mentioned this pull request Sep 8, 2019

Echo context leaking information across requests #1395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concurrency safe context option. #1158

Add concurrency safe context option. #1158

andreiavrammsd commented Jul 4, 2018 •

edited

Loading

codecov bot commented Jul 4, 2018 •

edited

Loading

stale bot commented Nov 30, 2018

andreiavrammsd commented Nov 30, 2018

alexaandru commented Feb 2, 2019

im-kulikov left a comment

andreiavrammsd commented Feb 2, 2019

im-kulikov commented Feb 2, 2019

im-kulikov commented Feb 2, 2019

alexaandru commented Feb 2, 2019

andreiavrammsd commented Feb 3, 2019

alexaandru commented Feb 3, 2019

andreiavrammsd commented Feb 8, 2019

alexaandru commented Feb 9, 2019

andreiavrammsd commented Feb 9, 2019

alexaandru commented Feb 9, 2019

im-kulikov left a comment

vishr commented Feb 9, 2019 •

edited

Loading

vishr commented Feb 10, 2019 •

edited

Loading

andreiavrammsd commented Feb 10, 2019

vishr commented Feb 11, 2019

alexaandru commented Feb 11, 2019

alexaandru commented Feb 11, 2019

vishr commented Feb 11, 2019

andreiavrammsd commented Feb 11, 2019

vishr commented Feb 11, 2019 •

edited

Loading

alexaandru commented Feb 11, 2019

vishr commented Feb 11, 2019 •

edited

Loading

alexaandru commented Feb 11, 2019

andreiavrammsd commented Feb 12, 2019

vishr commented Feb 12, 2019

andreiavrammsd commented Feb 12, 2019

Add concurrency safe context option. #1158

Add concurrency safe context option. #1158

Conversation

andreiavrammsd commented Jul 4, 2018 • edited Loading

codecov bot commented Jul 4, 2018 • edited Loading

Codecov Report

stale bot commented Nov 30, 2018

andreiavrammsd commented Nov 30, 2018

alexaandru commented Feb 2, 2019

im-kulikov left a comment

Choose a reason for hiding this comment

andreiavrammsd commented Feb 2, 2019

im-kulikov commented Feb 2, 2019

im-kulikov commented Feb 2, 2019

alexaandru commented Feb 2, 2019

andreiavrammsd commented Feb 3, 2019

alexaandru commented Feb 3, 2019

andreiavrammsd commented Feb 8, 2019

alexaandru commented Feb 9, 2019

andreiavrammsd commented Feb 9, 2019

alexaandru commented Feb 9, 2019

im-kulikov left a comment

Choose a reason for hiding this comment

vishr commented Feb 9, 2019 • edited Loading

vishr commented Feb 10, 2019 • edited Loading

andreiavrammsd commented Feb 10, 2019

vishr commented Feb 11, 2019

alexaandru commented Feb 11, 2019

alexaandru commented Feb 11, 2019

vishr commented Feb 11, 2019

andreiavrammsd commented Feb 11, 2019

vishr commented Feb 11, 2019 • edited Loading

alexaandru commented Feb 11, 2019

vishr commented Feb 11, 2019 • edited Loading

alexaandru commented Feb 11, 2019

andreiavrammsd commented Feb 12, 2019

vishr commented Feb 12, 2019

andreiavrammsd commented Feb 12, 2019

andreiavrammsd commented Jul 4, 2018 •

edited

Loading

codecov bot commented Jul 4, 2018 •

edited

Loading

vishr commented Feb 9, 2019 •

edited

Loading

vishr commented Feb 10, 2019 •

edited

Loading

vishr commented Feb 11, 2019 •

edited

Loading

vishr commented Feb 11, 2019 •

edited

Loading