separate handlers for concurrent handlers

[jehiah]

An unexpected api regression/change is that you can't set concurrent handlers to each use their own handler object. This was useful when you wanted separate state for each concurrent handler.

The current api doesn't allow you to call SetHandler or SetConcurrentHandlers multiple times, but i don't see a strict reason not to allow that. Thoughts @mreiferson

[mreiferson]

Yea, I experienced this while updating nsq_to_http and nsq_to_nsq for the new API. What I found was they didn't really have (or need) separate state per concurrent handler.

Taking a step back, the reason why I went with the current API is because I believe the best way to think about handlers is to treat them like an HTTP handler for a certain URL (topic). You're getting the same type of messages to the same instance of a handler. It doesn't matter how many concurrent requests are happening.

[jehiah]

I'll let you know what else i uncover but the primary use has been to give each handler unique ID's so that logging inside a handler has an id to prepend to output so that it's clear all the logs that came from a single handler sequence. I think that's a pretty legit use case and works as an example of something that shouldn't require hoops to go through to get 1:1 handler struct and handler functions.

[jehiah]

Looking through the code i don't see any reason not to just drop the check that guards against adding mutliple handlers. Calling multiple times wouldn't be anything different than SetConcurrentHandlers isn't already doing.

I think we can just drop:

    if atomic.LoadInt32(&r.runningHandlers) > 0 {
        panic("cannot call setHandlers() multiple times")
    }

[jehiah]

I'll mention that this also removes barriers to the transition to new API naming.

[mreiferson]

If we allow you to call this method more than once it should be prefixed with Add not Set to reflect those semantics.

But, I've already explained why I think it should be as it is. Think about how you would handle this if it were an HTTP handler. I think those semantics make sense here too, so I don't think we should make this change.

[jehiah]

#56 made me realize that another motivation for this change is that it would provide a way to use handlers that are not goroutine safe with concurrent handlers because this allows you to keep handler objects 1:1 w/ goroutines.

[mreiferson]

I agree that this is a good example of a place where explicitly documenting that message handler state is shared amongst goroutines and that it requires synchronization if modifying.

I'm not necessarily convinced that's a bad thing and that we need this API, though 😄.

But let's try to wrap this up...

I think we discussed offline at one point, my objection is the asymmetry in being able to add but not remove.

If we make a change such that calling AddHandler after any connect method will panic, then I think I'd be good to go here.

Agreed?

[mreiferson]

edit: and document when it can be used and that it will panic...

[jehiah]

Agreed. i'll update.