-
Notifications
You must be signed in to change notification settings - Fork 352
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
Update message writer and reader to ignore Message info from DI #3058
Update message writer and reader to ignore Message info from DI #3058
Conversation
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
@@ -904,24 +904,18 @@ private ODataMessageInfo GetOrCreateMessageInfo(Stream messageStream, bool isAsy | |||
{ | |||
if (this.messageInfo == null) | |||
{ | |||
if (this.serviceProvider == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, why do we still keep "https://github.com/OData/odata.net/blob/main/src/Microsoft.OData.Core/ODataServiceCollectionExtensions.cs#L52" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, that could be a breaking change. The likelihood of breaking existing code is low, likely 0, but here's sample code that will break today if we removed ODataMessageInfo
from the DI:
var serviceCollection = new ServiceCollection();
serviceCollection.AddDefaultODataServices();
var serviceProvider = serviceCollection.BuildServiceProvider();
var messageInfo = serviceProvider.GetRequiredService<ODataMessageInfo>();
In the current version, this would return a messageInfo instance, if we remove it from DI and ship a new version, then this code would suddenly throw an exception when the user updates.
If we want to be strict about non-breaking minor and patch releases, we can remove it from the next major version and consider making the class private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a breaking change but it seems it could have very few users since a search shows that this was only being used in that location. Also the DI version is scoped which should not reproduce this bug/issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/OData/odata.net/blob/main/buildandtest.yml#L24 this build step is the one that fails to build
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Show resolved
Hide resolved
test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs
Outdated
Show resolved
Hide resolved
var content1 = string.Concat(Enumerable.Repeat('A', 1000_000)); | ||
var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to create such long strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily but it would make the interleaving more apparent if it happens
Removes calls to GetRequiredService as these instances are overwritten immediately.
Simplifies their creation to call stacks where they are needed.