-
Notifications
You must be signed in to change notification settings - Fork 10.3k
MessagePack v2 causes TypeLoadExceptions in Microsoft.AspNetCore.SignalR.StackExchangeRedis #18074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@paul-pz could you provide more details on this? What exception is thrown from the runtime? What version of StackExchangeRedis are you using? |
MessagePack v2 has many (by design) breaking API changes from v1. Given ASP.NET Core now only runs on .NET Core, which doesn't support loading two assembly versions with the same simple name at once, this means if SignalR compiles against MessagePack v1 then it's toxic for a web app to reference MessagePack v2 since only one will load and cause a failure in one use or the other. |
We can look at updating to MessagePack v2 in a future major version. I'll put this in our queue to review for future planning. Until then, @AArnott is correct, using MessagePack v2 is not compatible with SignalR. I'll leave this open to track updating. |
arg :D, I was trying to attempt a PR to update @anurse |
It's not MessagePack v2 is incompatible with .NET Core, because it certainly is compatible. The problem is that .NET Core by default will only load one version of a given assembly (whereas .NET Framework was perfectly willing to load multiple versions). So if you load MessagePack v2, then ASP.NET Core's Signal library can't load v1 and will get v2 instead. Since v1 and v2 have API breaking changes, this breaks SignalR. If you are careful with your assembly load contexts, you can get .NET Core to load your MessagePack v2 code in an isolated assembly load context so that it doesn't disrupt SignalR's loading of MessagePack v1 in another assembly load context. This may work well if your messagepack code is fairly isolated. |
Any tips if the the submitted PR should do the trick if it's not API breaking change or "new feature" ? (Once i understand what i did wrong for the files from the SubModule -_-) |
@AArnott, yes that's correct - I'm using MessagePack directly. I upgraded to V2 via nuget. What I would have expected is that the upgrade should have failed because the SignalR Redis package should have an upper constraint < 2.0.0 (since its obviously incompatible). Instead it succeeds, builds and fails at runtime. |
I will add it to the current PR on |
That wasn't known at the time of release. We generally don't include upper bounds in our packages because they must be determined at the time of release and we do not know what the upper bound is at that time. Adding a upper bound means that even when a compatible release is made, it is not possible for a user to accept the update. Whereas, without an upper bound, if the release is compatible, the user can update. This is a general policy in ASP.NET Core though and it isn't really a good idea to discuss it in this thread. Let's stay focused on the task at hand: updating the packages to support MessagePack 2.0
FYI @tebeco we don't take major version updates to dependencies in servicing releases so there's no point in a PR to |
Yes, this is considered a breaking change. Users may have been getting MessagePack through our dependency and may have code using the 1.x APIs. We cannot break them in a non-major release and must consider a breaking change update to transitive dependencies as a breaking change ourselves. |
ahhhh i did not thought about client pulling |
I wonder if it's worth just sticking with the principles around semantic versioning and assume that a major version bump means a breaking API change? You're right, it may turn out that the major bump doesn't affect the API's being used - but the user can always force the installation of the package if they want to test this. I'd see this as a typical push left quality issue - go with stability and allow the user to accept the risk explicitly. It would also bring behavior more in line with package managers like NPM. |
Regardless of the general policy, we know now that messagepack v2 is breaking. So adding the upper bound in a servicing release makes perfect sense to me. |
I'd really prefer not to thrash the long-standing upper limit bounds discussion in this thread and keep this focused on updating to MessagePack v2 (to fix the actual underlying problem). Our current policy is to never apply upper bounds. I'm certainly not saying that policy is not open for discussion but I'd like to avoid bogging this specific change to update to MessagePack v2 (which is important) down in that (since it has historically been a long-discussed issue with a lot of valid viewpoints but it tends to drown out other discussions). The main work we can get quick and easy consensus on here is updating SignalR to use MessagePack 2.0 in the next major release. |
+1 for upating Microsoft.AspNetCore.SignalR.Protocols.MessagePack to use MessagePack > 2.0 😄 |
@valeriob tip: use the github reactions on the issue description to vote it up. |
This is done via #18133 |
Microsoft.AspNetCore.SignalR.StackExchangeRedis breaks at runtime if MessagePack v2 is added. Might be worth constraining the nuget package until this fixed.
The text was updated successfully, but these errors were encountered: