Skip to content
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

Migrate to ASP.NET "Bedrock" abstractions #5935

Merged
merged 2 commits into from
Sep 10, 2019

Conversation

ReubenBond
Copy link
Member

Deletes our copies of networking abstractions in favor of the shared Bedrock abstractions, in preparation for 3.0.

Ideally we could also remove SocketConnection, etc, but there are no publicly accessible versions of those yet so we still have a significant amount of duplicated code.

cc @davidfowl

@ReubenBond
Copy link
Member Author

image

This is a little concerning, maybe I have an assembly mismatch locally (running tests on .NET Framework)

@davidfowl
Copy link
Member

Any repro?

@ReubenBond
Copy link
Member Author

ReubenBond commented Sep 7, 2019

It reproduces on my office & home machines on both .NET Fx & .NET Core 2.0.
Running any test will repro it. eg, JSON_EchoJson.

This is quirky, VS is showing the exception fields under a field of type ReadOnlySequence<byte> :
image

The exception is thrown in user code from here:

called via its ctor from here:

var reader = new BinaryTokenStreamReader2(payloadBuffer);

@davidfowl
Copy link
Member

.NET Core 2.0? What about 2.1 (2.0 is out of support).

@ReubenBond
Copy link
Member Author

Repros on 2.1 & 2.2

@davidfowl
Copy link
Member

Pull together as minimal a repro as you can an file an issue

@ReubenBond
Copy link
Member Author

@davidfowl the bug was caused by an error in my code - in one place I was advancing the PipeReader before actually consuming the data. The fix is simple:

diff --git a/src/Orleans.Core/Networking/ConnectionPreamble.cs b/src/Orleans.Core/Networking/ConnectionPreamble.cs
index f3f552595..7635a69ad 100644
--- a/src/Orleans.Core/Networking/ConnectionPreamble.cs
+++ b/src/Orleans.Core/Networking/ConnectionPreamble.cs
@@ -96,8 +96,9 @@ int ReadLength(ref ReadOnlySequence<byte> b)
             }
 
             var payloadBuffer = buffer.Slice(0, length);
-            input.AdvanceTo(payloadBuffer.End);
 
+            try
+            {
                 var reader = new BinaryTokenStreamReader2(payloadBuffer);
                 var grainId = reader.ReadGrainId();
 
@@ -123,6 +124,11 @@ int ReadLength(ref ReadOnlySequence<byte> b)
                 }
 
                 return (grainId, protocolVersion, siloAddress);
+            }
+            finally
+            {
+                input.AdvanceTo(payloadBuffer.End);
+            }
 
             void CheckForCompletion(ref ReadResult r)
             {

@sergeybykov sergeybykov merged commit bf02ff2 into dotnet:master Sep 10, 2019
@ReubenBond ReubenBond deleted the migrate/to-bedrock branch September 10, 2019 15:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants