fix event concurrency flaw #2519

goolAdapter · 2018-09-29T08:41:40Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

codecov-io · 2018-09-29T09:31:57Z

Codecov Report

Merging #2519 into develop will decrease coverage by 0.07%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           develop    #2519      +/-   ##
===========================================
- Coverage    61.31%   61.24%   -0.08%     
===========================================
  Files          202      202              
  Lines        16710    16718       +8     
===========================================
- Hits         10246    10239       -7     
- Misses        5599     5611      +12     
- Partials       865      868       +3

Impacted Files	Coverage Δ
libs/events/events.go	`96.07% <78.57%> (-0.74%)`	⬇️
libs/clist/clist.go	`66.49% <0%> (-1.53%)`	⬇️
consensus/reactor.go	`71.18% <0%> (-1.43%)`	⬇️

melekes · 2018-10-01T08:03:18Z

libs/events/events_test.go

@@ -162,6 +166,52 @@ func TestAddDifferentListenerForDifferentEvents(t *testing.T) {
 	}
 }

+var stopInputEvent = false


can we not introduce global variables please

I'm sorry for dalayed reply，I'm taking a holiday, so the next relay may also delay.
Yes，this global variables will eliminated.

melekes · 2018-10-01T08:04:50Z

libs/events/events.go

 	evl.mtx.Lock()
 	defer evl.mtx.Unlock()

 	if evl.removed {
-		return
+		return fmt.Errorf("listener was removed.")


var ErrListenerRemoved = errors.New("listener was removed")

Maybe even a struct for it which holds info about the listener id?

that is ok， I'll replace other error message with named error too.

melekes · 2018-10-01T08:05:55Z

libs/events/events.go

@@ -168,10 +172,15 @@ func (cell *eventCell) RemoveListener(listenerID string) int {

 func (cell *eventCell) FireEvent(data EventData) {
 	cell.mtx.RLock()
+	var listenerCopy []EventCallback


what if we run each callback in a goroutine?

Goroutine per callback could work if we have more control for them than just fire-and-forget. We should evaluate lifecycles of all goroutines carefully.

yes， this previously can exist a deadlock， when we call add/remove event in callback. but it can work under carefully control，copy is a way to discard the burden.

I prefer the copy, it free the event user.

this previously can exist a deadlock

does the test below uncovers that too? as far as I can see we have a mutex here, so we don't need to make a copy

I'll create a new test to show the deadlock.

xla

Thanks for the contribution, just some minor style and language nits.

xla · 2018-10-02T13:55:40Z

libs/events/events.go

@@ -168,10 +172,15 @@ func (cell *eventCell) RemoveListener(listenerID string) int {

 func (cell *eventCell) FireEvent(data EventData) {
 	cell.mtx.RLock()
+	var listenerCopy []EventCallback


Goroutine per callback could work if we have more control for them than just fire-and-forget. We should evaluate lifecycles of all goroutines carefully.

xla · 2018-10-02T13:57:02Z

libs/events/events.go

@@ -58,7 +59,7 @@ func (evsw *eventSwitch) OnStart() error {

 func (evsw *eventSwitch) OnStop() {}

-func (evsw *eventSwitch) AddListenerForEvent(listenerID, event string, cb EventCallback) {
+func (evsw *eventSwitch) AddListenerForEvent(listenerID, event string, cb EventCallback) (err error) {


Let's avoid named returns when possible, any strong reason to have it here?

no strong reason, I'll change to the repository's convention.

xla · 2018-10-02T13:58:29Z

libs/events/events.go

 	evl.mtx.Lock()
 	defer evl.mtx.Unlock()

 	if evl.removed {
-		return
+		return fmt.Errorf("listener was removed.")


Maybe even a struct for it which holds info about the listener id?

melekes · 2018-10-03T06:41:25Z

libs/events/events.go

+}
+
+func (e ErrListenerWasRemoved) Error() string {
+	return fmt.Sprintf("listener %s was removed.", e.listener)


no dot (.) at the end please

Use https://github.com/golang/lint

melekes · 2018-10-03T06:41:59Z

libs/events/events.go

@@ -208,8 +216,9 @@ func (evl *eventListener) AddEvent(event string) error {
 	defer evl.mtx.Unlock()

 	if evl.removed {
-		return fmt.Errorf("listener was removed.")
+		return &ErrListenerWasRemoved{listener: evl.id}


No need for error to be a pointer (&)

melekes · 2018-10-03T06:44:40Z

libs/events/events_test.go

 	for i := 0; i < roundCount; i++ {
 		evsw.RemoveListener("listener")
 	}
 	done <- 1
 }

-func inputAddListenerForEvent(t *testing.T, evsw EventSwitch, done chan uint64) {
+func inputAddListenerForEvent(t *testing.T, evsw EventSwitch, done chan uint64, roundCount int, stopInputEvent *bool) {


can we use a channel instead of *bool for stopInputEvent

func inputAddListenerForEvent(t *testing.T, evsw EventSwitch, done chan uint64, roundCount int, stopInputEvent chan<- bool) { .... stopInputEvent <- true

chan is ok， but bool flag looks more simple，it's only used for reduce fail log.

melekes · 2018-10-05T09:18:51Z

libs/events/events.go

@@ -73,9 +81,13 @@ func (evsw *eventSwitch) AddListenerForEvent(listenerID, event string, cb EventC
 	}
 	evsw.mtx.Unlock()

+	var err error


err := below

melekes · 2018-10-05T09:19:54Z

libs/events/events_test.go

@@ -162,6 +166,52 @@ func TestAddDifferentListenerForDifferentEvents(t *testing.T) {
 	}
 }

+func inputRemoveListener(t *testing.T, evsw EventSwitch, done chan uint64, roundCount int) {


why input prefix? also done chan struct{} would be more appropriate, don't you think?

yes，done chan uint64 is inspired by other test case，here done chan struct{} is enough

melekes · 2018-10-05T09:20:35Z

libs/events/events_test.go

+
+func inputAddListenerForEvent(t *testing.T, evsw EventSwitch, done chan uint64, roundCount int, stopInputEvent *bool) {
+	for i := 0; i < roundCount; i++ {
+		index := i


no need in this extra variable

it need, the next event callback use index for log.

melekes · 2018-10-05T09:21:03Z

libs/events/events_test.go

+}
+
+func TestAddAndRemoveListenerConcurrency(t *testing.T) {
+	var stopInputEvent = false


can be united

var ( ... )

melekes · 2018-10-05T09:22:11Z

libs/events/events_test.go

+
+	evsw.RemoveListener("listener") // make sure remove last
+
+	for i := 0; i < roundCount && !stopInputEvent; i++ {


what happens if the test fails? panic? or this test uncovers the DATA race?

yes, this test is used to uncovers the DATA race

I'm back from a long vacation，sorry for the delayed response，I will fix the review issue.

melekes · 2018-10-08T05:51:14Z

CHANGELOG_PENDING.md

@@ -42,4 +42,10 @@ IMPROVEMENTS:
 BUG FIXES:
 - [autofile] \#2428 Group.RotateFile need call Flush() before rename (@goolAdapter)
 - [node] \#2434 Make node respond to signal interrupts while sleeping for genesis time
+- [consensus] [\#1690](https://github.com/tendermint/tendermint/issues/1690) wait for


where are these coming from? improper merge?

melekes · 2018-10-08T05:53:09Z

libs/events/events_test.go

+		for i := 0; i < roundCount; i++ {
+			evsw.RemoveListener("listener")
+		}
+		done1 <- struct{}{}


close(done1)

here chan can recycled automatically by the garbage collector， but I'll close it explicit

melekes · 2018-10-08T05:53:21Z

libs/events/events_test.go

+					stopInputEvent = true
+				})
+		}
+		done2 <- struct{}{}


close(done2)

here chan can recycled automatically by the garbage collector， but I'll close it explicit

xla

👍 🍡

Thanks for the follow-up!

goolAdapter added 2 commits September 29, 2018 16:37

fix event concurrency flaw

2790d05

modify changelog

01122fd

goolAdapter requested review from ebuchman, melekes and xla as code owners September 29, 2018 08:41

goolAdapter added 2 commits September 29, 2018 16:54

fix a mistake

6f10785

fix a lint issue

2268655

goolAdapter mentioned this pull request Sep 29, 2018

test_p2p: non-deterministic test fail #2520

Closed

modify changelog

8485e56

melekes reviewed Oct 1, 2018

View reviewed changes

xla suggested changes Oct 2, 2018

View reviewed changes

modify for review issue

e7f6efb

melekes reviewed Oct 3, 2018

View reviewed changes

melekes approved these changes Oct 5, 2018

View reviewed changes

melekes reviewed Oct 5, 2018

View reviewed changes

goolAdapter added 2 commits October 8, 2018 09:47

modify for review issue

e67cf61

merge from develop

914d5e5

melekes reviewed Oct 8, 2018

View reviewed changes

goolAdapter added 2 commits October 8, 2018 14:42

modify for review issue

86dbf38

merge from develop

a8d18b8

melekes approved these changes Oct 8, 2018

View reviewed changes

xla approved these changes Oct 8, 2018

View reviewed changes

xla merged commit 4b2bf02 into tendermint:develop Oct 8, 2018


		evsw.RemoveListener("listener") // make sure remove last

		for i := 0; i < roundCount && !stopInputEvent; i++ {

fix event concurrency flaw #2519

fix event concurrency flaw #2519

Conversation

goolAdapter commented Sep 29, 2018 • edited Loading

codecov-io commented Sep 29, 2018 • edited Loading

Codecov Report

melekes Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xla left a comment

Choose a reason for hiding this comment

goolAdapter commented Sep 29, 2018 •

edited

Loading

codecov-io commented Sep 29, 2018 •

edited

Loading

melekes Oct 1, 2018 •

edited

Loading

melekes Oct 1, 2018 •

edited

Loading