Make DefaultOptions a function that produces new Options on every call

[unkaktus]

At the moment DefaultOptions is a global struct that is being copied by value. Some of the fields are pointers, so they got copied as well while retaining their value and thus making these fields effectively global. This breaks logic of multiple operations with DefaultOptions which may be modified by other users (e.g. in tests).
This PR is making it a function that returns a fresh copy of DefaultOptions each time to use independently.

[coveralls]

Coverage increased (+0.03%) to 94.559% when pulling 29c0ba3 on nogoegst:default-options-func into 34c8842 on nats-io:master.

[tylertreat]

I'm in favor of this change, but it will require a major version bump since this would be a breaking change.

[kozlovic]

To mitigate the compatibility issue, we could keep the DefaultOptions as it is right now and simply add the DefaultOptions() function. We would have to make sure to use the function in our tests, etc.. and in general users should use the option, but at least that would not break existing code. What do you guys think?

[tylertreat]

@kozlovic I don't think you can have a var and func with the same name in the same package? But yes, we could name it something different.

[wallyqs]

Another idea maybe, how about deprecating the fields which are pointers as part of DefaultOptions? Seems it is only Dialer the one that wouldn't be a copy while others are safe to use.

[aricart]

How about NewDefaultOptions(), and keep the struct the same.

[kozlovic]

or GetDefaultOptions() like we have in NATS Streaming server.

[unkaktus]

Another idea maybe, how about deprecating the fields which are pointers as part of DefaultOptions? Seems it is only Dialer the one that wouldn't be a copy while others are safe to use.

@wallyqs, it does not feel like a good idea to me. You can't guarantee that all fields are primitive and do not contain any pointers.
Actually I got faced this issue while fixing TLS. tls.Config is full of pointers and other magic things. I don't think we can drop support for arbitrary fields just to work around broken API.

Personally I think it's better to drop deprecated API faster while bumping major version. Otherwise it will become a big mess where no one understands how it works (or will just keep going without fixing their code).

[wallyqs]

@nogoegst but tls.Config is not part of nats.DefaultOptions so sharing of tls.Config accidentally by calling opts := nats.DefaultOptions several times wouldn't be possible no?

[unkaktus]

@wallyqs tls.Config is in the Options instance of which DefaultOptions is. This just defaults to nil. By now.

[kozlovic]

@nogoegst I am torn. Although I fully agree with you that using DefaultOptions as a struct may lead to errors, especially with the presence of pointers (there is already one, and others may be added in the future), making this change would break potentially lots of users (arguably, they should vendor if they are concerned about breaking changes). The worse, is that the use of DefaultOptions was safe until 1.2.2, where the pointer was added.

Leaving DefaultOptions and adding GetDefaultOptions() would avoid breaking users' existing code base. I was thinking that we could remove the Dialer from DefaultOptions structure, add a comment that DefaultOptions is deprecated and users should use GetDefaultOptions() instead, and make sure that nothing is ever added to DefaultOptions structure itself, but instead in GetDefaultOptions().

Removing the Dialer from DefaultOptions is not going to break users (the ones at 1.2.2) since the Connect() call is using the exact same net.Dialer with DefaultTimeout if no Dialer is set in Options.

I can do all these changes myself, but I wanted to give you the opportunity to discuss or implement those changes since you have originally submitted the PR. Let me know how you want to proceed.

[unkaktus]

@kozlovic agreed. Fixed by renaming to GetDefaultOptions().

[coveralls]

Coverage increased (+0.03%) to 94.559% when pulling 12ef716 on nogoegst:default-options-func into 34c8842 on nats-io:master.

[kozlovic]

Thanks for your contribution!