search

microsoft / Microsoft.IO.RecyclableMemoryStream

A library to provide pooling for .NET MemoryStream objects to improve application performance.
MIT License
1.98k stars 205 forks source link

Breaking changes at 3.0.0 version break OfficeOpenXml NuGet package #324

Closed birbilis closed 10 months ago

birbilis commented 10 months ago

Please see https://github.com/dotnet/Open-XML-SDK/issues/1638

benmwatson commented 10 months ago

Is there a specific proposal in mind?

With major version changes, breaking changes are expected. They happen all the time with most libraries.

That project will need to be rebuilt with v3.0 or stay on v2.x to maintain compatibility.

pentp commented 10 months ago

Some types of breaking changes are expected (like removing obsolete types/methods), but not these - the changed return type on RecyclableMemoryStreamManager.GetStream methods will cause a dll hell where it's impossible to upgrade from 2.x to 3.x without recompiling and updating all packages that depend on Microsoft.IO.RecyclableMemoryStream at the same time.

Which in turn means that no-one can upgrade to 3.x because they would cause a binary breaking change for all their downstream dependencies. So OfficeOpenXml (or anything else that depends on Microsoft.IO.RecyclableMemoryStream) will basically be forever locked to 2.x version.

sungam3r commented 9 months ago

Which in turn means that no-one can upgrade to 3.x .... will basically be forever locked to 2.x version.

Not true. Major/binary changes will always be major/binary. So it has always been and will always be with absolutely any packages and their dependencies. RMS 3.0 changes is not any special situation here. Life goes on.

birbilis commented 9 months ago

Staying with old versions of code that no one maintains is a recipe for future disasters security-wise Plus at some point some other package will require the new version of the RecyclableMemoryStream and cause a deadlock since the OpenXml one doesn't seem to close its issues list fast enough

sungam3r commented 9 months ago

Staying with old versions of code that no one maintains ....

изображение

Nevertheless I still don't understand what you expect/suggest and what are you complaining about. Do not make breaking changes at all? As @benmwatson said

With major version changes, breaking changes are expected. They happen all the time with most libraries.

pentp commented 9 months ago

Nevertheless I still don't understand what you expect/suggest and what are you complaining about. Do not make breaking changes at all? As @benmwatson said

With major version changes, breaking changes are expected. They happen all the time with most libraries.

I'm suggesting avoiding binary breaking changes for non-obsolete APIs that are in active use. Yes, there are different kinds of breaking changes happening with major releases, but a ABI break for a core API will cause an extremely painful DLL hell (potentially even bifurcation - there have been examples where some packages had a major ABI break and it took many years and uncountable number of lost brain cells before it was finally forgotten).

An ABI break falls in bucket 1 according to .NET rules: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-changes.md

As for the specific API RecyclableMemoryStreamManager.GetStream, I'm suggesting reverting the return type change.

birbilis commented 9 months ago

Btw, where is the original rationale recorded for those breaking changes that shows the clear benefits they bring compared to the issues they can cause for users? Do you wake up one day and say let's break some API?

sungam3r commented 9 months ago

Planned here - #258

sungam3r commented 9 months ago

Actually #165 was issued in June 2021 and @benmwatson carefully postponed it for v3.

benmwatson commented 9 months ago

The type change was to remedy a long-standing problem with many users needing to cast the returned MemoryStream back to RecyclableMemoryStream. The original design was to completely abstract that away, as this is meant to be a drop-in replacement for MemoryStream, however I think this was a mistake, as enough functionality diverges that the API should give direct access to the RMS without additional casting.

I understand it's a major breaking change, but this library is only getting more popular, and it would be better to suffer the pain sooner rather than later and correct the original deficiency rather than live with it indefinitely.

Also note, that this library is not part of the .NET core libraries and does not adhere to the same standards for breaking changes, nor necessarily follows any other processes specific to that project.

birbilis commented 9 months ago

thanx for the extra info on the rationale, seems issue was with EPPlus (was confused since it is using OfficeOpenXml namespace for its ExcelPackage class). They seem to have fixed it with latest EPPlus NuGet package release (7.0.8)

as for missing constructors wonder if some compatibility optional package could have been released to host them (thought it might be more of a hack to pull it out transparently)