nats-io / nats.net.v1

The official C# Client for NATS
Apache License 2.0
646 stars 154 forks source link

Be careful about breaking binary compatibility #851

Closed Denrask closed 5 months ago

Denrask commented 8 months ago

Proposed change

Hi nats.net team!

Thank you for creating a well functioning library!

I've been using it for over a year and its been a joy. Only thing is that I'd like you to consider binary compatibility more in the future. I'm assuming you're following semantic versioning and I've noticed breaking changes in patch and minor updates. For example, adding default overloads between 1.0.0 and 1.0.8 in GetDefaultOptions caused a binary compatibility break:

https://github.com/nats-io/nats.net/compare/1.0.0...1.0.8#diff-6b8a73f2145bd833d10b2398f64094d94e31d483e6b366fa78242fbdaa0fa1c2R111

Also, the minor update from 1.0.8 to 1.1.1 adding default overloads to CreateConnection constructor causes binary compatibility break:

https://github.com/nats-io/nats.net/compare/1.0.8...1.1.1#diff-6b8a73f2145bd833d10b2398f64094d94e31d483e6b366fa78242fbdaa0fa1c2R46

I'd just like to let you know that this can be a significant pain point for users and kindly ask of you to consider adding constructors or new methods instead of adding default overloads, despite the potential harm to code readability.

Thank you very much and happy holidays!

Use case

The general motivation is just to inform the team of a code style that might bring frustration to some users that rely on binary compatibility.

Contribution

I'd absolutely contribute code if this was something I could fix, but this is merely a suggestion of something to be aware of. :+1:

scottf commented 8 months ago

Are you saying that adding a parameter with a default value as the last parameter in the signature caused your code not to compile? Or is this an issue when you put the new version of the library with(?) a pre-existing compiled binary.

Denrask commented 8 months ago

Thank you for responding, @scottf !

No, the code will still compile.

Or is this an issue when you put the new version of the library with(?) a pre-existing compiled binary.

This exactly. If you compile against a constructor or method without the default overload and then switch to a new assembly with one, you get a missing method exception.

scottf commented 8 months ago

I've done this in more than one release and it has never been brought up. I understood it was okay as long as it didn't break the compile and behavior was not different. I'll ask around to team to see where this falls on semver. If it breaks it then it won't happen again.

scottf commented 8 months ago

@Denrask This is a message I posted to the client developers. I referenced this thread then added:

This was accomplished by adding a parameter with a default value to an api. Old code compiles fine. So when a developer is using our library directly, this really is a non-issue, just compile your code and your binary is fine. The problem is when a dev uses library Z and Z uses the Nats client. This means that the dev cannot upgrade Nats without upgrading Z, and they have no control of Z.

So the ship has already sailed on current .NET. It has been done multiple times over the 3 years I've been here and this is the first complaint we've gotten. But now that we are aware of it, going forward, should we ensure this never happens again?

sixlettervariables commented 8 months ago

The .NET team has/had an analyzer for this sort of thing. With libraries I think this is particularly important, and I'm sorry I missed this if I reviewed the work.

Binary compat is nice to have so in place upgrades of components can occur without a recompile such as when out-of-band security patches are needed. A specific example being, NATS is used in a product I worked on that is installed on high security, air gapped networks, with significant burdens for updates. The lower the impact, the better in that environment.

Denrask commented 8 months ago

Thank you @scottf !

Binary compat is nice to have so in place upgrades of components can occur without a recompile such as when out-of-band security patches are needed. A specific example being, NATS is used in a product I worked on that is installed on high security, air gapped networks, with significant burdens for updates. The lower the impact, the better in that environment.

Yes @sixlettervariables , this is indeed an excellent reason.

I think the reason that we've experienced this so much, is that we are using a plugin architecture. More and more of our plugins start to utilize NATS. Essentially, each of these plugins brings their own dependencies. Which means we end up with multiple NATS client libraries in the same "installation". Our resolver picks the latest version from these NATS clients, but the plugin which used the older version is now throwing exceptions due to binary incompatibility.

I realize that many application types are not affected by binary incompatibility, but this issue was just to make you aware that you have users exists that do deeply care. Thank you very much for listening and responding!