Allow use of a default MemoryPoolFeature on ConnectionContext #18579

Ninds · 2020-01-25T12:14:24Z

This facilitates the use of other Transports other than sockets.
When kestrel bound to MemoryTransport (Bedrock davidfowl/BedrockFramework#65) a NullReferenceException is thrown since the MemoryPoolFeature is not set on the ConnectionContext set in that Transport

Summary of the changes (Less than 80 chars)

Detail 1
Detail 2

Addresses #bugnumber (in this specific format)

davidfowl · 2020-01-25T12:24:05Z

src/Servers/Kestrel/Core/src/Middleware/HttpConnectionMiddleware.cs

@@ -33,7 +34,7 @@ public Task OnConnectionAsync(ConnectionContext connectionContext)
                Protocols = _protocols,
                ServiceContext = _serviceContext,
                ConnectionFeatures = connectionContext.Features,
-                MemoryPool = memoryPoolFeature.MemoryPool,
+                MemoryPool = memoryPoolFeature?.MemoryPool ?? System.Buffers.MemoryPool<byte>.Shared,


We should let this be null (without the null ref that is). Don't fallback to the default memory pool though. If that breaks we should fix those places that use the pool directly.

We also need to add a test for this to make sure it doesn't break.

I see what you mean. So leave it as
memoryPool = memoryPoolFeature?.MemoryPool
and solve any side-effects later on in the code path where they occur ?
This could be tested with MemoryEndPoint.

Isn't the MemoryPoolFeature accessible in any middleware wired up to Ketsrel through the HttpConnectionContext ? If so, any middleware assuming it's presence will break when the endpoint in the instance of Kestrel is changed to a non-socket based endpoint like MemoryEndPoint.
Consider the scenario where my kestrel based app,, is switching to MemoryEndpoint for testing purposes and at the same time I am retrieving the MemoryPoolFeature in my middleware making use of it.

Yes but this isn't a required feature so I'd like to understand the places that rely on it being non-null.

Why not include ?? MemoryPool<byte>.Shared here? The same thing will happen implicitly later when we new up PipeOptions.

Yes but this isn't a required feature so I'd like to understand the places that rely on it being non-null.

By adding ?? MemoryPool<byte>.Shared, the middleware is no longer relying on the feature.

What middleware is that? This is a specific middleware (the HTTP middleware) that relies on it currently. The TLS middleware already handles this.

Before doing this fix I just want to understand what code in the HTTP layer relies on this being non-null. I have no problem putting it back.

What's the point? Like you say, this is the HTTP middleware. By adding this, there would be no code in the HTTP layer that relies on this being non-null.

I just want to understand what code in the HTTP layer relies on this being non-null
I have no problem putting it back

Would I be right in thinking that these two actions are independent and that one is not a blocker for the other ? I guess the first can be achieved by setting the MemoryPool to null and seeing where the Tests fail ( I did this and there were at least 13 failures)
and the second can be done to introduce the fix but without any significant hindrance to carrying out the first (given the change is trivial) ?

if on the other hand :

I just want to understand what code in the HTTP layer relies on this being non-null

is likely to lead to a different outcome then that's a different matter.

A bit of context (no pun intended), the MemoryPool was set in the ConnectionContext when binding to a socket but was not set in the ConnectionContext when binding to a (Bedrock) MemoryEndpoint. In both cases ConnectionContext was intended to pushed down the HttpMiddleware.

Is the Transport factory obliged to set the MemoryPool ? If so, how could it be enforced ? If not then I guess provision of a fallback default is mandatory and why is it set in the ConnectionContext in kestrel ?

@halter73

What's the point? Like you say, this is the HTTP middleware. By adding this, there would be no code in the HTTP layer that relies on this being non-null.

The transport isn't required to set a pool and the IMemoryPoolFeature isn't required. Kestrel needs to take that into account. Kestrel can also polyfill by defaulting to using a pool but I have 2 concerns:

We don't have an understanding where it does that. I'd like to understand before making assumptions.

We probably want to use the SlabMemoryPool and not the default memory pool in the places where we explicitly allocate.

@Ninds

Would I be right in thinking that these two actions are independent and that one is not a blocker for the other ? I guess the first can be achieved by setting the MemoryPool to null and seeing where the Tests fail ( I did this and there were at least 13 failures)
and the second can be done to introduce the fix but without any significant hindrance to carrying out the first (given the change is trivial) ?

The code change is trivial, measuring the impact isn't without understanding what parts of the code rely on this instance.

Is the Transport factory obliged to set the MemoryPool ? If so, how could it be enforced ? If not then I guess provision of a fallback default is mandatory and why is it set in the ConnectionContext in kestrel ?

Features can be optional, it's part of the system. It's not set on the ConnectionContext, it's just an optional feature that middleware may use if it exists.

gfoidl · 2020-01-27T09:01:58Z

src/Servers/Kestrel/Core/src/Middleware/HttpConnectionMiddleware.cs

@@ -43,5 +44,6 @@ public Task OnConnectionAsync(ConnectionContext connectionContext)

            return connection.ProcessRequestsAsync(_application);
        }
+


Nit: remove this empty line.

analogrelay · 2020-02-19T22:23:32Z

Sounds like we're waiting for author feedback on the proposal to continue to let the memory pool be null but change our consumption points to use a default. Correct? Trying to identify what we need to do to get this PR resolved/closed.

Ninds · 2020-02-21T16:51:06Z

I will try this weekend to figure out where the memorypool is being used.

shirhatti · 2020-04-10T20:26:01Z

@davidfowl Do you still want this?

davidfowl · 2020-04-10T20:27:06Z

Yes.

halter73 · 2020-04-15T20:31:47Z

@davidfowl Are you fine with this as is?

davidfowl · 2020-04-15T20:37:04Z

At this point the only change I would make is to use the SlabPool.

halter73 · 2020-04-15T21:07:26Z

At this point the only change I would make is to use the SlabPool.

I think that makes more sense. I figured that given a transport that doesn't implement the IMemoryPoolFeature, we don't care about using the most optimal pool. But then again, why not?

We can just change System.Buffers.MemoryPool<byte>.Shared to SlabMemoryPoolFactory.Create(). We should also change Http3ConnectionMiddleware to do the same thing.

Edit: SlabMemoryPool is shared source, so we'll also need to add <Compile Include="$(SharedSourceRoot)Buffers.MemoryPool\*.cs" LinkBase="MemoryPool" /> to Kestrel.Core.csproj.

halter73 · 2020-04-15T21:27:48Z

Looking at this more, I think changing to use the SlabMemoryPool in Kestrel.Core will be very difficult. There are a bunch of test projects that have InternalsVisibleTo access to both Kestrel.Core internals and the internals to one of the transports that also use the SlabMemoryPool as shared source.

This means adding the SlabMemoryPool to Kestrel.Core causes a bunch of type conflicts 😢

halter73 · 2020-04-15T21:57:24Z

After more discussion with @davidfowl, it looks like we're taking this as is 😄

…set on ConnectionContext This facilitates the use of other Transports other than sockets. When kestrel bound to MemoryTransport (Bedrock davidfowl/BedrockFramework#65) a NullReferenceException is thrown since the MemoryPoolFeature is not set on the ConnectionContext set in that Transport

dnfclas · 2020-04-15T22:09:00Z

All CLA requirements met.

halter73 · 2020-04-15T22:18:32Z

@dotnet/dnceng Is the CLA necessary for such a small change? I don't know why the bot is only triggering now.

halter73 · 2020-05-15T21:43:09Z

Thanks @Ninds!

Ninds requested review from analogrelay, halter73, jkotalik and Tratcher as code owners January 25, 2020 12:14

ghost added the area-servers label Jan 25, 2020

davidfowl reviewed Jan 25, 2020

View reviewed changes

gfoidl reviewed Jan 27, 2020

View reviewed changes

analogrelay added this to the 5.0.0-preview1 milestone Jan 27, 2020

analogrelay modified the milestones: 5.0.0-preview1, 5.0.0-preview2 Feb 19, 2020

halter73 approved these changes Apr 15, 2020

View reviewed changes

halter73 self-requested a review April 15, 2020 21:07

halter73 closed this Apr 15, 2020

halter73 reopened this Apr 15, 2020

Ninds and others added 2 commits April 15, 2020 15:05

fixup

ef978e4

halter73 force-pushed the master branch from f1d8925 to ef978e4 Compare April 15, 2020 22:08

halter73 approved these changes Apr 15, 2020

View reviewed changes

pranavkm removed this from the 5.0.0-preview2 milestone Apr 15, 2020

pranavkm added this to the 5.0.0-preview4 milestone Apr 15, 2020

analogrelay modified the milestones: 5.0.0-preview4, 5.0.0-preview5 Apr 20, 2020

BrennanConroy modified the milestones: 5.0.0-preview5, 5.0.0-preview6 May 15, 2020

BrennanConroy assigned halter73 May 15, 2020

halter73 merged commit f908247 into dotnet:master May 15, 2020

Ninds deleted the master branch February 3, 2022 21:52

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

		@@ -43,5 +44,6 @@ public Task OnConnectionAsync(ConnectionContext connectionContext)

		return connection.ProcessRequestsAsync(_application);
		}

Allow use of a default MemoryPoolFeature on ConnectionContext #18579

Allow use of a default MemoryPoolFeature on ConnectionContext #18579

Uh oh!

Conversation

Ninds commented Jan 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ninds Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ninds Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ninds Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Feb 19, 2020

Uh oh!

Ninds commented Feb 21, 2020

Uh oh!

shirhatti commented Apr 10, 2020

Uh oh!

davidfowl commented Apr 10, 2020

Uh oh!

halter73 commented Apr 15, 2020

Uh oh!

davidfowl commented Apr 15, 2020

Uh oh!

halter73 commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Apr 15, 2020

Uh oh!

halter73 commented Apr 15, 2020

Uh oh!

dnfclas commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

halter73 commented Apr 15, 2020

Uh oh!

halter73 commented May 15, 2020

Uh oh!

Uh oh!

Ninds Jan 25, 2020 •

edited

Loading

Ninds Jan 26, 2020 •

edited

Loading

Ninds Jan 28, 2020 •

edited

Loading

halter73 commented Apr 15, 2020 •

edited

Loading

dnfclas commented Apr 15, 2020 •

edited

Loading