Topic handler bug fixes #225
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aschmahmann
force-pushed
the
fix/224
branch
2 times, most recently
from
c57bc57
to
96c5d3f
Compare
aschmahmann
commented
Nov 1, 2019
Stebalien
approved these changes
Nov 1, 2019
topic.go
Outdated
| if t.closed { | ||
| return nil | ||
| } | ||
|
|
||
| req := &rmTopicReq{t, make(chan error, 1)} | ||
| t.p.rmTopic <- req |
There was a problem hiding this comment.
This was true for all previous PubSub functions and the new Topic ones. I've pushed an update, lmk if that's what you had in mind.
Three questions about fixing this:
- Do I want custom
ErrPubSubClosedmessages or is the context error enough (both for the PubSub functions and for the Topic functions)? - Should I set
Topic.closed=trueon any/all of the pubsub context errors? - It seems more reasonable to return an empty array then nil for
GetTopicsandListPeersright?
There was a problem hiding this comment.
@vyzo any thoughts on the first 2 questions here? Would like to get this pushed out ASAP since the Close bug is particularly bad.
There was a problem hiding this comment.
- either works, it's a very small thing.
- probably not, I don't see what we would gain from this.
vyzo
approved these changes
Nov 3, 2019
…een cancelled instead of hanging
This was referenced Nov 4, 2019
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the issues named in #224 namely:
Calling Topic.Close() throws an NRE 😨
Calling Topic.Close() still allows some calls to be utilized (e.g. can still Publish after Close)