Use examined rather than consumed for content length of body. #8223

jkotalik · 2019-03-06T03:28:42Z

I still think I need #8198 to resolve this.

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

Tratcher · 2019-03-07T18:12:08Z

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

            _context.Input.AdvanceTo(consumed, examined);

-            OnDataRead(dataLength);
+            var newlyExamined = examinedLength - _totalExaminedInPreviousReadResult;
+            if (newlyExamined > 0)


Did you confirm if examined can go backwards? Or does Advance throw?

Advance doesn't throw.

Examined can't go backwards.

jkotalik · 2019-03-08T00:36:11Z

Current test failures are due to how we now handle Read/Advance combinations. If we reached the end of content, we make ReadAsync return immediately with the current ReadResult, even if the user didn't call advance. I can fix this by adding a bool that flips each time read/advance is called, but I don't think it's a huge deal. It's inconsistent with how pipes work, but IMO isn't a big deal.

I'll add a bool for now and let you all decide how it looks.

jkotalik · 2019-03-08T22:15:25Z

@aspnet/brt
[xUnit.net 00:01:21.57] E2ETests.PublishAndRunTests.PublishAndRun_Test(variant: Server: Kestrel, TFM: netcoreapp3.0, Type: Portable, Arch: x64) [FAIL]

Locked files...

jkotalik · 2019-03-08T22:16:42Z

@aspnet/brt

Microsoft (R) Test Execution Command Line Tool Version 16.0.0-preview-20190124-02
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
[xUnit.net 00:00:01.44]     Microsoft.AspNetCore.Identity.InMemory.Test.InMemoryStoreTest.ChangeEmailTokensFailsAfterEmailChanged [FAIL]
Failed   Microsoft.AspNetCore.Identity.InMemory.Test.InMemoryStoreTest.ChangeEmailTokensFailsAfterEmailChanged
Error Message:
 System.Security.Cryptography.CryptographicException : An error occurred while trying to encrypt the provided data. Refer to the inner exception for more information.
---- System.IO.IOException : The process cannot access the file '/home/helixbot/.aspnet/DataProtection-Keys/key-bd22c8d4-ce56-4de9-9cca-5fc1eeb7bc21.xml' because it is being used by another process.
Stack Trace:
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Protect(Byte[] plaintext) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs:line 141
   at Microsoft.AspNetCore.Identity.DataProtectorTokenProvider`1.GenerateAsync(String purpose, UserManager`1 manager, TUser user) in /_/src/Identity/Core/src/DataProtectorTokenProvider.cs:line 86
   at Microsoft.AspNetCore.Identity.Test.UserManagerSpecificationTestBase`2.ChangeEmailTokensFailsAfterEmailChanged() in /_/src/Identity/Specification.Tests/src/UserManagerSpecificationTests.cs:line 1761
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at System.IO.FileStream.Init(FileMode mode, FileShare share, String originalPath)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
   at System.IO.File.OpenRead(String path)
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.ReadElementFromFile(String fullPath) in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 102
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.GetAllElementsCore()+MoveNext() in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 83
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.GetAllElements() in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 68
   at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.GetAllKeys() in /_/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs:line 152
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.CreateCacheableKeyRingCore(DateTimeOffset now, IKey keyJustAdded) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 57
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.ICacheableKeyRingProvider.GetCacheableKeyRing(DateTimeOffset now) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 254
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.GetCurrentKeyRingCore(DateTime utcNow) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 186
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.GetCurrentKeyRing() in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 142
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Protect(Byte[] plaintext) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs:line 102
Results File: /home/helixbot/dotnetbuild/work/b32e663f-b42f-40af-be42-bd070ee33b2a/Work/b35bd579-11f7-4641-93d6-5e8831a65951/Exec/TestResults/_dnfed28op000AZM_2019-03-08_11_08_50_913.trx

jkotalik · 2019-03-13T04:40:24Z

@halter73 @davidfowl same with this one

halter73 · 2019-03-14T22:46:44Z

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

+
+            if (_readCompleted)
+            {
+                _readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted);


Suggested change

_readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted);

_readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), Interlocked.Exchange(ref _userCanceled, 0) == 1, _readCompleted);

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

src/Servers/Kestrel/Core/test/MessageBodyTests.cs

halter73

I'm mostly concerned about the _finalAdvanceAfterReadingEntireContentLength AdvanceTo logic.

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs

halter73 · 2019-03-15T23:25:11Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs

+                Assert.Equal(5, readResult.Buffer.Length);
+                httpContext.Request.BodyReader.AdvanceTo(readResult.Buffer.Start, readResult.Buffer.End);
+                readResult = await httpContext.Request.BodyReader.ReadAsync();
+                Assert.Equal(5, readResult.Buffer.Length);


Do you have any tests that attempt more than one read after getting a completed read result?

halter73

If you don't have any tests that attempt more than one read after observing a completed read result, add one.

jkotalik requested review from davidfowl and Tratcher March 6, 2019 03:28

jkotalik added this to the 3.0.0-preview4 milestone Mar 6, 2019

davidfowl reviewed Mar 6, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Outdated Show resolved Hide resolved

pakrym reviewed Mar 6, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Outdated Show resolved Hide resolved

Eilon added the area-servers label Mar 6, 2019

jkotalik force-pushed the jkotalik/bufferRequestBody branch from f9fc8e2 to 21a57c8 Compare March 6, 2019 23:39

Tratcher reviewed Mar 7, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Outdated Show resolved Hide resolved

Tratcher reviewed Mar 7, 2019

View reviewed changes

jkotalik marked this pull request as ready for review March 8, 2019 22:21

analogrelay added the brt-review-needed label Mar 9, 2019

jkotalik closed this Mar 12, 2019

jkotalik reopened this Mar 12, 2019

jkotalik force-pushed the jkotalik/bufferRequestBody branch from 3f3a591 to 2cdb6da Compare March 12, 2019 18:07

analogrelay removed the brt-review-needed label Mar 12, 2019

jkotalik requested a review from halter73 March 13, 2019 04:40

halter73 reviewed Mar 14, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Outdated Show resolved Hide resolved

halter73 reviewed Mar 14, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Outdated Show resolved Hide resolved

halter73 reviewed Mar 14, 2019

View reviewed changes

src/Servers/Kestrel/Core/test/MessageBodyTests.cs Outdated Show resolved Hide resolved

halter73 requested changes Mar 14, 2019

View reviewed changes

halter73 reviewed Mar 15, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs Show resolved Hide resolved

halter73 reviewed Mar 15, 2019

View reviewed changes

halter73 approved these changes Mar 15, 2019

View reviewed changes

jkotalik added 2 commits March 18, 2019 11:41

Need a similar solution that http2 has

c3556da

Prototype of fix

62cbe32

jkotalik added 7 commits March 18, 2019 11:41

I hate this solution with a passion but whatever

b963405

bodyReader

781f611

Make Read/Advance throw appropriately

6cd36ef

Set isReading in one more place

6d09e14

React to corefx changes

e43f889

Feedback

6eaeb7e

Feedback:

14af05d

jkotalik force-pushed the jkotalik/bufferRequestBody branch from ec3fd76 to 14af05d Compare March 18, 2019 20:44

fb

6c9b43e

jkotalik merged commit 26c487b into master Mar 19, 2019

jkotalik deleted the jkotalik/bufferRequestBody branch March 19, 2019 15:26

analogrelay mentioned this pull request Mar 25, 2019

Allow more granular control over the ApplicationMaxBufferSize #5299

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use examined rather than consumed for content length of body. #8223

Use examined rather than consumed for content length of body. #8223

Uh oh!

jkotalik commented Mar 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tratcher Mar 7, 2019

Uh oh!

jkotalik Mar 8, 2019

Uh oh!

davidfowl Mar 12, 2019

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 13, 2019

Uh oh!

halter73 Mar 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 left a comment

Uh oh!

Uh oh!

halter73 Mar 15, 2019

Uh oh!

halter73 left a comment

Uh oh!

Uh oh!

	_readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), isCanceled: false, _readCompleted);
	_readResult = new ReadResult(_readResult.Buffer.Slice(consumed, _readResult.Buffer.End), Interlocked.Exchange(ref _userCanceled, 0) == 1, _readCompleted);

Use examined rather than consumed for content length of body. #8223

Use examined rather than consumed for content length of body. #8223

Uh oh!

Conversation

jkotalik commented Mar 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tratcher Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 8, 2019

Uh oh!

jkotalik commented Mar 13, 2019

Uh oh!

halter73 Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halter73 Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!