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

Subscribe("foo", nil) inconsistency between Conn and EncodedConn #48

Closed
rbucker opened this issue Feb 1, 2015 · 6 comments
Closed

Subscribe("foo", nil) inconsistency between Conn and EncodedConn #48

rbucker opened this issue Feb 1, 2015 · 6 comments

Comments

@rbucker
Copy link

rbucker commented Feb 1, 2015

EncodedConn.Subscribe("foo", nil) throws a nil pointer exception instead of deleting the subscription as documented in the README.

Conn.Subscribe("foo", nil) returns normally but does not delete the subscription.

Since there is a Subscription.Unsubscribe() then this 'nil' behavior should be avoided and either return an error in the error field or throw an exception. Looking into the Subscribe code there is some code that looks for a nil callback and some that throws an exception when reflecting.

@derekcollison
Copy link
Member

This in the README correct?

@derekcollison
Copy link
Member

Send a PR.. ;)

@derekcollison
Copy link
Member

Will take a look when I have some time.. Thanks..

@rbucker rbucker changed the title Code sample in the readme Subscribe("foo", nil) inconsistency between Conn and EncodedConn Feb 1, 2015
@rbucker
Copy link
Author

rbucker commented Feb 1, 2015

I found this comment in the code:
If no MsgHandler is given, the subscription is a synchronous subscription and can be polled via Subscription.NextMsg().

@derekcollison
Copy link
Member

A normal connection with no handler creates a synchronous subscriber, however will look at the EncodedConn version more closely..

@derekcollison
Copy link
Member

EncodedConn is not built currently for NextMsg, I will add an err return though.

derekcollison added a commit that referenced this issue Sep 18, 2015
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

2 participants