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.

Sign up for GitHub

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

Jump to bottom

Fix unsubscribe #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix unsubscribe #29

wants to merge 2 commits into from

Conversation

nothize
Copy link
Contributor

@nothize nothize commented Jul 15, 2017

Fix unsubscribe all and unmatched bug that removed other observers

Use a more efficient way to construct the new subscribe list while
fixing the bug.

A test case is included as an earlier commit so that the bug can be reproduced by running the test case:

$ GATEWAY_URL=localhost:4003 go test -run TestUnsubscribe
=== RUN   TestUnsubscribeAllAndUnmatched
--- FAIL: TestUnsubscribeAllAndUnmatched (0.10s)
	engine_test.go:72: created engine for reuse
	engine_test.go:158: rc should be unsubscribed from allObservers
	engine_test.go:167: rc should be unsubscribed from unObservers
FAIL
exit status 1
FAIL	github.com/gofinance/ib	0.113s

@nothize
Add test case for unsubscribe all and unmatched
1d3aa70 
This reveals a bug that when there is more than 1 subscribers, the
end result will always contain the observer to be removed.

In general, where n is the length of the subscriber list
	when n = 1, works as expected
	when n > 1, subscriber list = [observer to be removed] x (n-1)
@nothize
Fix unsubscribe all and unmatched bug that removed other observers
914cb6f 
Use a more efficient way to construct the new subscribe list while
fixing the bug.
@nothize
Copy link
Contributor Author

nothize commented Jan 26, 2019

@creack @indrekj @joelreymont @uwe

Please review, thanks!

@creack
Copy link
Contributor

creack commented Jan 28, 2019

I'll give it a shot over the weekend.

@creack creack self-requested a review January 31, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@creack creack Awaiting requested review from creack

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nothize @creack