unintuitive behavior when using &nsq.Config{}

[mreiferson]

I got this on Ubuntu 12.04, go 1.2

panic: non-positive interval for NewTicker

goroutine 141 [running]:
runtime.panic(0x815520, 0xc21010d230)
    /usr/local/go/src/pkg/runtime/panic.c:266 +0xb6
time.NewTicker(0x0, 0x3fd8a050fdd8d7f5)
    /usr/local/go/src/pkg/time/tick.go:22 +0x9f
github.com/bitly/go-nsq.(*Consumer).lookupdLoop(0xc21005a8c0)
    /tmp/godep/rev/2b/9a8b5edc036235580256e9a5e5f118a0025151/src/github.com/bitly/go-nsq/consumer.go:228 +0x6e
created by github.com/bitly/go-nsq.(*Consumer).ConnectToNSQLookupd
    /tmp/godep/rev/2b/9a8b5edc036235580256e9a5e5f118a0025151/src/github.com/bitly/go-nsq/consumer.go:216 +0x282

This is 2b9a8b5

I'm connecting to 8 nsqlookupd hosts (which might increase the chance of this happening)

I'm not setting config.lookupdPollInterval in my code.

[kjk]

Ok, it was most likely caused by me passing &nsq.Config{} to NewConsumer() and NewProducer().

Changing it to use nsq.NewConfig() fixed this.

Nevertheless, this is a bad way to fail.

If &nsq.Config{} is not a valid config value, then some validation should take place and an error returned from NewConsumer() and NewProducer() (instead of panic() or some other bad behavior later on).

Alternatively, &nsq.Config{} could have been silently "upgraded" by NewConsumer() and NewProducer() to have the same values as NewConfig().

I understand it might be tempting to brush this off as "read documentation" but people (including myself) don't alway read documentation from start to end and constructing objects with literal syntax is an idiom.

[mreiferson]

these are reasonable comments, let me think about it a bit and we'll figure out how to improve this...

[mreiferson]

I think my preference is to make it such that any operation on nsq.Config (Set, Get, or passing it into New{Consumer,Producer}) will result in it being initialized to its defaults before proceeding.

Thoughts @kjk?

cc @jehiah

[kjk]

sounds good to me

[mreiferson]

RFR

[jehiah]

LGTM (aside from jenkins failing). Just add some comments in initialize() describing why it's there instead of in a New... function.

[dmitshur]

If &nsq.Config{} is not a valid config value, what about making its type unexported, so that the only way for external packages to create a (valid) config value is via func NewConfig() *config {. If one wants to make a custom config, they would have exactly one way to do it (since &nsq.config{} would not compile):

cfg := nsq.NewConfig()
cfg.Set("max_backoff_duration", 0)

Was this idea considered, and if so, is there a reason why it would be worse/inappropriate?

[mreiferson]

I thought about it, but I imagined that there might be use cases where you want to pass around an nsq.Config but you would have no way to declare any function that accepted it (because it's private).

I do prefer solutions that result in there being only one correct way to do things if we can come up with one.

For the record, as far as I'm concerned, everything in master is fair game for improving the API before a stable release is tagged, so thanks for your feedback.

[dmitshur]

I thought about it, but I imagined that there might be use cases where you want to pass around an nsq.Config but you would have no way to declare any function that accepted it (because it's private).

Yeah, that's a valid concern (the only drawback that I could think of so far).

One way to fix that problem that would be to declare an exported interface that *config implements, e.g.:

type Config interface {
    Set(option string, value interface{}) error
}

func NewConfig() Config {
    return &config{
        // default setting here
    }
}

This is possible in your case since all fields in current Config struct are unexported and are only accessible to outsiders via methods.

[mreiferson]

Yea, I thought of that too, but didn't further consider it because I was concerned about any other type being able to satisfy the interface and be able to be passed in without type safety.

BUT, I forgot that you can do the old un-exported method on an exported interface trick to prevent outsiders from satisfying it:

type Config interface {
    Set(string, interface{}) error
    sentinel()
}

type config struct {
    // ...
}

func (c *config) sentinel() {}

What do you think?

[dmitshur]

It's funny that we have to do quite a few extra things to achieve the two goals:

• Make nsq.NewConfig() the only way create a nsq config.
• Allow the result of nsq.NewConfig() to be passed around or kept inside a user struct.

But yeah, I think what you propose would allow that to work.

It's still worth it to do extra work internal to your library if that allows you to make the public API better/cleaner.

The users would have to change their internal instances of *nsq.Config into nsq.Config, since it'll be an interface rather than a pointer to a struct.