-
Notifications
You must be signed in to change notification settings - Fork 92
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
Pubsub is broken due to Timeout => LastCritical change #39
Comments
…not for ReadResp (fixes #39) Without this distinction the pubsub package (and any pubsub package which uses ReadResp) would be completely borked. At the same time we still need to consider timeouts critical for actual command/response situations (as explained in 041a70a). So now only ReadResp will not set LastCritical for timeouts, but others will.
Damn, that was a huge oversight on my part, I'm really sorry about that. I've just pushed a branch which should fix the problems while keeping backwards compat for everything except the I'll let you take a look at the change before I go and merge it in, 2fb934f |
And thanks for pointing this out and digging into it as much as you did. I didn't quite go with your solution, but I definitely appreciate the effort you put into debugging it :) |
Your solution looks great. It'll certainly work for my team! Thank you for getting on it so quick. This issue is my first real foray into contributing feedback to the open source community, so I was trying to make sure I got it right before spouting off. It makes me unreasonably happy that it was helpful and appreciated. |
…not for ReadResp (fixes #39) Without this distinction the pubsub package (and any pubsub package which uses ReadResp) would be completely borked. At the same time we still need to consider timeouts critical for actual command/response situations (as explained in 041a70a). So now only ReadResp will not set LastCritical for timeouts, but others will.
The change that was made doesn't just set LastCritical as the pull request states - it also closes the connection:
This doesn't just break pubsub for implementations using LastCritical as the comment states, it breaks any pubsub implementation that uses a timeout, doesn't it?
The following sample code from the pubsub doc should no longer work:
On a timeout, LastCritical is set and the connection gets closed and we continue, at the start of the loop we call subc.Receive() which errors out because we attempted to read from a closed connection.
I think this change should be reversed, the problem it was trying to solve could be handled by the programmer who can evaluate how critical a timeout is to them - meanwhile pubsub is now severely handicapped because it cannot be used in conjunctions with timeouts. However, if the functionality is critical perhaps there can be a configuration option that can be set to make timeouts no longer critical (timeouts = critical by default, but the pubsub wrapper sets them to not-critical.)
Thank you.
The text was updated successfully, but these errors were encountered: