From 78fd2d3fe4406a2fd58899bd30a83808e4a5456d Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Mon, 9 Sep 2024 17:52:18 +0300 Subject: [PATCH 1/9] Update message writer and reader to ignore Message info from DI as it is overwritten. --- .../ODataMessageReader.cs | 28 ++--- .../ODataMessageWriter.cs | 37 +++--- .../MessageWriterConcurrencyTests.cs | 112 ++++++++++++++++++ 3 files changed, 139 insertions(+), 38 deletions(-) create mode 100644 test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs diff --git a/src/Microsoft.OData.Core/ODataMessageReader.cs b/src/Microsoft.OData.Core/ODataMessageReader.cs index 5b66f67bdb..00338fa61c 100644 --- a/src/Microsoft.OData.Core/ODataMessageReader.cs +++ b/src/Microsoft.OData.Core/ODataMessageReader.cs @@ -904,24 +904,18 @@ private ODataMessageInfo GetOrCreateMessageInfo(Stream messageStream, bool isAsy { if (this.messageInfo == null) { - if (this.serviceProvider == null) + this.messageInfo = new ODataMessageInfo { - this.messageInfo = new ODataMessageInfo(); - } - else - { - this.messageInfo = this.serviceProvider.GetRequiredService(); - } - - this.messageInfo.Encoding = this.encoding; - this.messageInfo.IsResponse = this.readingResponse; - this.messageInfo.IsAsync = isAsync; - this.messageInfo.MediaType = this.contentType; - this.messageInfo.Model = this.model; - this.messageInfo.PayloadUriConverter = this.payloadUriConverter; - this.messageInfo.ServiceProvider = this.serviceProvider; - this.messageInfo.MessageStream = messageStream; - this.messageInfo.PayloadKind = this.readerPayloadKind; + Encoding = this.encoding, + IsResponse = this.readingResponse, + IsAsync = isAsync, + MediaType = this.contentType, + Model = this.model, + PayloadUriConverter = this.payloadUriConverter, + ServiceProvider = this.serviceProvider, + MessageStream = messageStream, + PayloadKind = this.readerPayloadKind + }; } return this.messageInfo; diff --git a/src/Microsoft.OData.Core/ODataMessageWriter.cs b/src/Microsoft.OData.Core/ODataMessageWriter.cs index aa3ade2f22..fef522f3a2 100644 --- a/src/Microsoft.OData.Core/ODataMessageWriter.cs +++ b/src/Microsoft.OData.Core/ODataMessageWriter.cs @@ -19,9 +19,9 @@ namespace Microsoft.OData using Microsoft.OData.Metadata; #endregion Namespaces -/// -/// Writer class used to write all OData payloads (entries, resource sets, metadata documents, service documents, etc.). -/// + /// + /// Writer class used to write all OData payloads (entries, resource sets, metadata documents, service documents, etc.). + /// #if NETCOREAPP public sealed class ODataMessageWriter : IDisposable, IAsyncDisposable #else @@ -1111,7 +1111,7 @@ private ODataPayloadKind VerifyCanWriteValue(object value) // We cannot use ODataRawValueUtils.TryConvertPrimitiveToString for all cases since binary values are // converted into unencoded byte streams in the raw format // (as opposed to base64 encoded byte streams in the ODataRawValueUtils); see OIPI 2.2.6.4.1. - return value is byte[] ? ODataPayloadKind.BinaryValue : ODataPayloadKind.Value; + return value is byte[]? ODataPayloadKind.BinaryValue : ODataPayloadKind.Value; } /// @@ -1227,23 +1227,18 @@ private ODataMessageInfo GetOrCreateMessageInfo(Stream messageStream, bool isAsy { if (this.messageInfo == null) { - if (this.serviceProvider == null) - { - this.messageInfo = new ODataMessageInfo(); - } - else - { - this.messageInfo = this.serviceProvider.GetRequiredService(); - } - this.messageInfo.Encoding = this.encoding; - this.messageInfo.IsResponse = this.writingResponse; - this.messageInfo.IsAsync = isAsync; - this.messageInfo.MediaType = this.mediaType; - this.messageInfo.Model = this.model; - this.messageInfo.PayloadUriConverter = this.payloadUriConverter; - this.messageInfo.ServiceProvider = this.serviceProvider; - this.messageInfo.MessageStream = messageStream; + this.messageInfo = new ODataMessageInfo + { + Encoding = this.encoding, + IsResponse = this.writingResponse, + IsAsync = isAsync, + MediaType = this.mediaType, + Model = this.model, + PayloadUriConverter = this.payloadUriConverter, + ServiceProvider = this.serviceProvider, + MessageStream = messageStream + }; } return this.messageInfo; @@ -1321,7 +1316,7 @@ private async Task WriteToOutputAsync(ODataPayloadKind payload this.outputContext = await this.format.CreateOutputContextAsync( this.GetOrCreateMessageInfo(messageStream, true), this.settings).ConfigureAwait(false); - + return await writeFunc(this.outputContext).ConfigureAwait(false); } } diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs new file mode 100644 index 0000000000..46483699e9 --- /dev/null +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -0,0 +1,112 @@ +//--------------------------------------------------------------------- +// +// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information. +// +//--------------------------------------------------------------------- + +using System.Collections.Generic; +using System.IO; +using System.Threading.Tasks; +using System; +using Xunit; +using Microsoft.Extensions.DependencyInjection; +using System.Linq; + +namespace Microsoft.OData.Core.Tests +{ + public class MessageWriterConcurrencyTests + { + /// + /// Verifies that concurrent message writer does not interleave execution and isolates the underlying streams. + /// + /// A task for the asyncronous test + + [Fact] + public async Task VerifyConcurrentResultsAreConsistentAsync() + { + ServiceCollection services = new(); + services.AddDefaultODataServices(); + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + await Task.CompletedTask; + var content1 = string.Concat(Enumerable.Repeat('A', 1000_000)); + var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); + for (int i = 0; i < 1000; i++) + { + var values = await Task.WhenAll([WritePayload(content1, serviceProvider), WritePayload(content2, serviceProvider)]); + Assert.Equal(content1.Length, values[0].Length); + Assert.Equal(content2.Length, values[1].Length); + + Assert.Equal(content1, values[0]); + Assert.Equal(content2, values[1]); + } + } + + + /// + /// A helper function that writes to a strem using the message writer and returns the content that is present in the stream. + /// + /// String content to write. + /// A service provider with the default configurations. + /// A task that resolves to the string present in the output stream. + private async Task WritePayload(string content, IServiceProvider serviceProvider) + { + using Stream outputStream = new MemoryStream(); + + var message = new ODataMessage(outputStream, serviceProvider); + await using ODataMessageWriter writer = new ODataMessageWriter(message); + await Task.Yield(); + + await writer.WriteValueAsync(content); + + outputStream.Position = 0; + using var reader = new StreamReader(outputStream); + await Task.Yield(); + string writen = await reader.ReadToEndAsync(); + await writer.DisposeAsync(); + return writen; + } + + + class ODataMessage : IODataResponseMessage, IODataResponseMessageAsync, IServiceCollectionProvider + { + private Dictionary _headers = new(); + private Stream _outputStream; + public ODataMessage(Stream outputStream, IServiceProvider serviceProvider) + { + this.ServiceProvider = serviceProvider; + _outputStream = outputStream; + } + public IEnumerable> Headers => _headers; + + public int StatusCode { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } + + public IServiceProvider ServiceProvider { get; private set; } + + public string GetHeader(string headerName) + { + if (_headers.TryGetValue(headerName, out var value)) + { + return value; + } + + return null; + } + + public Stream GetStream() + { + return _outputStream; + } + + public Task GetStreamAsync() + { + return Task.FromResult(_outputStream); + } + + public void SetHeader(string headerName, string headerValue) + { + _headers[headerName] = headerValue; + } + } + } +} From 20cf1e103f58a6974cc3b0654d38602e67915eb9 Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Mon, 9 Sep 2024 19:44:54 +0300 Subject: [PATCH 2/9] Update test from pr review --- .../Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index 46483699e9..e5818e7018 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -28,7 +28,6 @@ public async Task VerifyConcurrentResultsAreConsistentAsync() services.AddDefaultODataServices(); ServiceProvider serviceProvider = services.BuildServiceProvider(); - await Task.CompletedTask; var content1 = string.Concat(Enumerable.Repeat('A', 1000_000)); var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); for (int i = 0; i < 1000; i++) From ebdb79cf1d5464a6d3c87dbf76c9121770f62dbd Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Mon, 9 Sep 2024 21:11:08 +0300 Subject: [PATCH 3/9] Add fix for older dotnet versions which are failing in the tests. --- .../MessageWriterConcurrencyTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index e5818e7018..0b99c28ca9 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -28,6 +28,7 @@ public async Task VerifyConcurrentResultsAreConsistentAsync() services.AddDefaultODataServices(); ServiceProvider serviceProvider = services.BuildServiceProvider(); + await Task.CompletedTask; // Added due to dotnet < 5 as async await cannot be used only in a loop var content1 = string.Concat(Enumerable.Repeat('A', 1000_000)); var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); for (int i = 0; i < 1000; i++) @@ -54,13 +55,16 @@ private async Task WritePayload(string content, IServiceProvider service var message = new ODataMessage(outputStream, serviceProvider); await using ODataMessageWriter writer = new ODataMessageWriter(message); + await Task.Yield(); await writer.WriteValueAsync(content); outputStream.Position = 0; using var reader = new StreamReader(outputStream); + await Task.Yield(); + string writen = await reader.ReadToEndAsync(); await writer.DisposeAsync(); return writen; From af4bcb07f5a12cf885b0523c041fabf2e10e4016 Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Tue, 10 Sep 2024 10:19:46 +0300 Subject: [PATCH 4/9] Update code with pr review suggestions. --- .../MessageWriterConcurrencyTests.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index 0b99c28ca9..87d6d790ee 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -33,7 +33,7 @@ public async Task VerifyConcurrentResultsAreConsistentAsync() var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); for (int i = 0; i < 1000; i++) { - var values = await Task.WhenAll([WritePayload(content1, serviceProvider), WritePayload(content2, serviceProvider)]); + var values = await Task.WhenAll([WritePayloadAsync(content1, serviceProvider), WritePayloadAsync(content2, serviceProvider)]); Assert.Equal(content1.Length, values[0].Length); Assert.Equal(content2.Length, values[1].Length); @@ -49,12 +49,13 @@ public async Task VerifyConcurrentResultsAreConsistentAsync() /// String content to write. /// A service provider with the default configurations. /// A task that resolves to the string present in the output stream. - private async Task WritePayload(string content, IServiceProvider serviceProvider) + private static async Task WritePayloadAsync(string content, IServiceProvider serviceProvider) { using Stream outputStream = new MemoryStream(); var message = new ODataMessage(outputStream, serviceProvider); - await using ODataMessageWriter writer = new ODataMessageWriter(message); + var responseMessage = new ODataResponseMessage(message, writing: true, enableMessageStreamDisposal: true, maxMessageSize: -1); + await using ODataMessageWriter writer = new ODataMessageWriter(responseMessage); await Task.Yield(); From 841a97849d3064ab3fdd0f2cccdde5206e24589c Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Tue, 10 Sep 2024 10:41:24 +0300 Subject: [PATCH 5/9] Update code from pr review. --- .../MessageWriterConcurrencyTests.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index 87d6d790ee..fe1728112d 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -19,10 +19,9 @@ public class MessageWriterConcurrencyTests /// /// Verifies that concurrent message writer does not interleave execution and isolates the underlying streams. /// - /// A task for the asyncronous test - + /// A task for the asynchronous test [Fact] - public async Task VerifyConcurrentResultsAreConsistentAsync() + public async Task VerifyConcurrentResultsAreIsolatedAsync() { ServiceCollection services = new(); services.AddDefaultODataServices(); @@ -44,7 +43,7 @@ public async Task VerifyConcurrentResultsAreConsistentAsync() /// - /// A helper function that writes to a strem using the message writer and returns the content that is present in the stream. + /// A helper function that writes to a stream using the message writer and returns the content that is present in the stream. /// /// String content to write. /// A service provider with the default configurations. @@ -66,9 +65,9 @@ private static async Task WritePayloadAsync(string content, IServiceProv await Task.Yield(); - string writen = await reader.ReadToEndAsync(); + string written = await reader.ReadToEndAsync(); await writer.DisposeAsync(); - return writen; + return written; } From fd4870dd3900f9a65858141086aa0f0b77046797 Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Tue, 10 Sep 2024 12:48:17 +0300 Subject: [PATCH 6/9] Update code from pr review. --- .../MessageWriterConcurrencyTests.cs | 51 +++---------------- 1 file changed, 7 insertions(+), 44 deletions(-) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index fe1728112d..35f842ab54 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -4,13 +4,13 @@ // //--------------------------------------------------------------------- -using System.Collections.Generic; using System.IO; using System.Threading.Tasks; using System; using Xunit; using Microsoft.Extensions.DependencyInjection; using System.Linq; +using Microsoft.OData.Tests; namespace Microsoft.OData.Core.Tests { @@ -52,7 +52,12 @@ private static async Task WritePayloadAsync(string content, IServiceProv { using Stream outputStream = new MemoryStream(); - var message = new ODataMessage(outputStream, serviceProvider); + var message = new InMemoryMessage + { + Stream = outputStream, + ServiceProvider = serviceProvider + }; + var responseMessage = new ODataResponseMessage(message, writing: true, enableMessageStreamDisposal: true, maxMessageSize: -1); await using ODataMessageWriter writer = new ODataMessageWriter(responseMessage); @@ -69,47 +74,5 @@ private static async Task WritePayloadAsync(string content, IServiceProv await writer.DisposeAsync(); return written; } - - - class ODataMessage : IODataResponseMessage, IODataResponseMessageAsync, IServiceCollectionProvider - { - private Dictionary _headers = new(); - private Stream _outputStream; - public ODataMessage(Stream outputStream, IServiceProvider serviceProvider) - { - this.ServiceProvider = serviceProvider; - _outputStream = outputStream; - } - public IEnumerable> Headers => _headers; - - public int StatusCode { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } - - public IServiceProvider ServiceProvider { get; private set; } - - public string GetHeader(string headerName) - { - if (_headers.TryGetValue(headerName, out var value)) - { - return value; - } - - return null; - } - - public Stream GetStream() - { - return _outputStream; - } - - public Task GetStreamAsync() - { - return Task.FromResult(_outputStream); - } - - public void SetHeader(string headerName, string headerValue) - { - _headers[headerName] = headerValue; - } - } } } From 7797ab14316fe056a95a18ea89efe1efda0304aa Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Wed, 11 Sep 2024 17:17:10 +0300 Subject: [PATCH 7/9] Update code from pr review. --- .../Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index 35f842ab54..f4e01a6907 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -50,7 +50,7 @@ public async Task VerifyConcurrentResultsAreIsolatedAsync() /// A task that resolves to the string present in the output stream. private static async Task WritePayloadAsync(string content, IServiceProvider serviceProvider) { - using Stream outputStream = new MemoryStream(); + await using Stream outputStream = new MemoryStream(); var message = new InMemoryMessage { From d64e6a3fdab52343c999ca4ccec431faabcc3594 Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Mon, 23 Sep 2024 12:28:44 +0300 Subject: [PATCH 8/9] Update step order --- buildandtest.yml | 12 ++++++------ .../MessageWriterConcurrencyTests.cs | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/buildandtest.yml b/buildandtest.yml index 0796247d81..03db3b3133 100644 --- a/buildandtest.yml +++ b/buildandtest.yml @@ -20,11 +20,6 @@ steps: inputs: version: '8.x' -# .NET Core App 2.1 is required to run the ESRP code signing task -- task: UseDotNet@2 - inputs: - version: '2.1.x' - - task: DotNetCoreCLI@2 displayName: 'Build' inputs: @@ -48,4 +43,9 @@ steps: command: 'test' arguments: '--configuration $(buildConfiguration) --collect "Code coverage"' projects: | - $(Build.SourcesDirectory)\test\EndToEndTests\Tests\Client\Microsoft.OData.Client.E2E.Tests\Microsoft.OData.Client.E2E.Tests.csproj \ No newline at end of file + $(Build.SourcesDirectory)\test\EndToEndTests\Tests\Client\Microsoft.OData.Client.E2E.Tests\Microsoft.OData.Client.E2E.Tests.csproj + +# .NET Core App 2.1 is required to run the ESRP code signing task +- task: UseDotNet@2 + inputs: + version: '2.1.x' \ No newline at end of file diff --git a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs index f4e01a6907..c1154059f6 100644 --- a/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs +++ b/test/FunctionalTests/Microsoft.OData.Core.Tests/MessageWriterConcurrencyTests.cs @@ -27,7 +27,6 @@ public async Task VerifyConcurrentResultsAreIsolatedAsync() services.AddDefaultODataServices(); ServiceProvider serviceProvider = services.BuildServiceProvider(); - await Task.CompletedTask; // Added due to dotnet < 5 as async await cannot be used only in a loop var content1 = string.Concat(Enumerable.Repeat('A', 1000_000)); var content2 = string.Concat(Enumerable.Repeat('B', 1000_000)); for (int i = 0; i < 1000; i++) From 072ad4768375e47373129a021d092fb1ea3d8ef8 Mon Sep 17 00:00:00 2001 From: David Wambugu Date: Mon, 23 Sep 2024 12:47:49 +0300 Subject: [PATCH 9/9] Move Esrp code dotnet target close to usage --- buildandtest.yml | 5 ----- nightly.yml | 5 ++++- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/buildandtest.yml b/buildandtest.yml index 03db3b3133..976e497b3f 100644 --- a/buildandtest.yml +++ b/buildandtest.yml @@ -44,8 +44,3 @@ steps: arguments: '--configuration $(buildConfiguration) --collect "Code coverage"' projects: | $(Build.SourcesDirectory)\test\EndToEndTests\Tests\Client\Microsoft.OData.Client.E2E.Tests\Microsoft.OData.Client.E2E.Tests.csproj - -# .NET Core App 2.1 is required to run the ESRP code signing task -- task: UseDotNet@2 - inputs: - version: '2.1.x' \ No newline at end of file diff --git a/nightly.yml b/nightly.yml index e4bd6bddca..f9b940ceba 100644 --- a/nightly.yml +++ b/nightly.yml @@ -11,7 +11,10 @@ BuildDropPath: '$(Build.ArtifactStagingDirectory)\$(nugetArtifactsDir)\sbom' - template: credscan.yml - + # .NET Core App 2.1 is required to run the ESRP code signing task + - task: UseDotNet@2 + inputs: + version: '2.1.x' # CodeSign - task: EsrpCodeSigning@1 displayName: 'ESRP CodeSign - OData'