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

lib's event implement have some flaw #2518

Closed
goolAdapter opened this issue Sep 29, 2018 · 2 comments
Closed

lib's event implement have some flaw #2518

goolAdapter opened this issue Sep 29, 2018 · 2 comments

Comments

@goolAdapter
Copy link
Contributor

goolAdapter commented Sep 29, 2018

This issue doesn't exist in the current runtime, but as an base lib, I think it's better to more robust.

Lib event designed for multi routine, but AddListenerForEvent and RemoveListener manipulate listener and eventCell in different order.

// Add event and listener
eventCell.AddListener(listenerID, cb)
listener.AddEvent(event)

// Remove callback for each event.
listener.SetRemoved()
for _, event := range listener.GetEvents() {
evsw.RemoveListenerForEvent(event, listenerID)
}

consider two routine,A call AddListenerForEvent and B call RemoveListener simultaneous.

B -> listener.SetRemoved()
A -> eventCell.AddListener(listenerID, cb)
A -> listener.AddEvent(event)
B -> for _, event := range listener.GetEvents() {
		evsw.RemoveListenerForEvent(event, listenerID)
}

At this time, Listener added by A will not be clean up. the fix is to try AddEvent first, only when AddEvent succeeded, eventCell can AddListener.

by the way,eventCell.FireEvent is easy to deadlock, as lock's range is uncontrollable when run callback.

func (cell *eventCell) FireEvent(data EventData) {
cell.mtx.RLock()
for _, listener := range cell.listeners {
listener(data)
}
cell.mtx.RUnlock()
}

@melekes
Copy link
Contributor

melekes commented Oct 5, 2018

note: we'll get rid of the events package in #1692

@xla
Copy link
Contributor

xla commented Oct 8, 2018

Merged to develop.

@xla xla closed this as completed Oct 8, 2018
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

3 participants