Pubsub is broken due to Timeout => LastCritical change

[bwparker]

The change that was made doesn't just set LastCritical as the pull request states - it also closes the connection:

if r.IsType(IOErr) {
        c.LastCritical = r.Err
        c.Close()
    }

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:

for {
    r := subc.Receive()
    if r.Timeout() {
        continue 
    } else if r.Err != nil {
         kv["err"] = r.Err
    }
}

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.

[mediocregopher]

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 LastCritical field (which, as I noted in the other commit, is barely used). I also added a test for this, the pubsub tests.... have not aged well let's say. That's something I really need to take care of.

I'll let you take a look at the change before I go and merge it in, 2fb934f0

[mediocregopher]

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 :)

[bwparker]

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.