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

Add mutex for protect Context.Keys map #1391

Merged
merged 7 commits into from Mar 16, 2020
Merged

Conversation

AcoNCodes
Copy link
Contributor

We get a panic in a loaded project related with use c.Get / c.Set in many middleware routines.

@codecov
Copy link

codecov bot commented Jun 12, 2018

Codecov Report

Merging #1391 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1391   +/-   ##
=======================================
  Coverage   98.47%   98.47%           
=======================================
  Files          41       41           
  Lines        2293     2298    +5     
=======================================
+ Hits         2258     2263    +5     
  Misses         20       20           
  Partials       15       15           
Impacted Files Coverage Δ
context.go 98.06% <100.00%> (+0.02%) ⬆️
gin.go 99.13% <100.00%> (ø)

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 67008be...b1d9b1a. Read the comment docs.

@thinkerou
Copy link
Member

thinkerou commented Jun 12, 2018

Hi @AcoNCodes alike pull request have #702 and issue #700

@night-codes
Copy link

@thinkerou this solution more lightweight, focused and without conflicts. The problem exists and forces me to use a fork that does not contain the latest updates. I'm looking forward to solve the problem.

@thinkerou
Copy link
Member

@night-codes thanks, I don't have any advice, the feature need @javierprovecho and @appleboy agree.

@duyanghao
Copy link

duyanghao commented Aug 30, 2019

any update for this pr?

appleboy
appleboy previously approved these changes Sep 2, 2019
@appleboy
Copy link
Member

appleboy commented Sep 2, 2019

@AcoNCodes Please fix the conflicts.

@appleboy appleboy added this to the 1.5 milestone Sep 2, 2019
@appleboy appleboy added the bug label Sep 3, 2019
@thinkerou
Copy link
Member

@AcoNCodes why not use sync.Map? thanks!

@thinkerou thinkerou modified the milestones: 1.5, 1.6 Oct 31, 2019
@appleboy
Copy link
Member

@thinkerou Performance: sync.Mutex > sync.Map for multiple reads and writes.

var s sync.RWMutex
var w sync.WaitGroup

func main() {
	mapTest()
	syncMapTest()
}
func mapTest() {
	m := map[int]int {1:1}
	startTime := time.Now().Nanosecond()
	w.Add(1)
	go writeMap(m)
	w.Add(1)
	go writeMap(m)

	w.Wait()
	endTime := time.Now().Nanosecond()
	timeDiff := endTime-startTime
	fmt.Println("map:",timeDiff)
}

func writeMap (m map[int]int) {
	defer w.Done()
	i := 0
	for i < 10000 {
		s.Lock()
		m[1]=1
		s.Unlock()
		i++
	}
}

func readMap (m map[int]int) {
	defer w.Done()
	i := 0
	for i < 10000 {
		s.RLock()
		_ = m[1]
		s.RUnlock()
		i++
	}
}

func syncMapTest() {
	m := sync.Map{}
	m.Store(1,1)
	startTime := time.Now().Nanosecond()
	w.Add(1)
	go writeSyncMap(m)
	w.Add(1)
	go writeSyncMap(m)

	w.Wait()
	endTime := time.Now().Nanosecond()
	timeDiff := endTime-startTime
	fmt.Println("sync.Map:",timeDiff)
}

func writeSyncMap (m sync.Map) {
	defer w.Done()
	i := 0
	for i < 10000 {
		m.Store(1,1)
		i++
	}
}

func readSyncMap (m sync.Map) {
	defer w.Done()
	i := 0
	for i < 10000 {
		m.Load(1)
		i++
	}
}

@appleboy appleboy merged commit 73ccfea into gin-gonic:master Mar 16, 2020
pull bot pushed a commit to scope-demo/gin that referenced this pull request Mar 16, 2020
* Add mutex for protect Context.Keys map

* Fix tests

Co-authored-by: Nikolay Tolkachov <nik.tolkachov@gmail.com>
Co-authored-by: Bo-Yi Wu <appleboy.tw@gmail.com>
byebyebruce pushed a commit to byebyebruce/gin that referenced this pull request Mar 25, 2020
* Add mutex for protect Context.Keys map

* Fix tests

Co-authored-by: Nikolay Tolkachov <nik.tolkachov@gmail.com>
Co-authored-by: Bo-Yi Wu <appleboy.tw@gmail.com>
@appleboy
Copy link
Member

appleboy commented May 3, 2020

@AcoNCodes The PR trigger the performance issue in #2350

@thinkerou
Copy link
Member

We get a panic in a loaded project related with use c.Get / c.Set in many middleware routines.

add a switch to open/close the feature? @appleboy

@appleboy
Copy link
Member

appleboy commented May 3, 2020

See #2351 I add new variable to enable/disable the mutex for protecting Context.Keys map (default as false) and also fix the comment.

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

Successfully merging this pull request may close these issues.

None yet

6 participants