Skip to content

Allow use of a default MemoryPoolFeature on ConnectionContext #18579

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 15, 2020
Merged

Allow use of a default MemoryPoolFeature on ConnectionContext #18579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
771d922
Allow use of a default MemoryPoolFeature when IMemoryPoolFeature not …
Ninds Jan 25, 2020
ef978e4
fixup
halter73 Apr 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Middleware/Http3ConnectionMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public Task OnConnectionAsync(MultiplexedConnectionContext connectionContext)
ConnectionContext = connectionContext,
ServiceContext = _serviceContext,
ConnectionFeatures = connectionContext.Features,
MemoryPool = memoryPoolFeature.MemoryPool,
MemoryPool = memoryPoolFeature?.MemoryPool ?? System.Buffers.MemoryPool<byte>.Shared,
LocalEndPoint = connectionContext.LocalEndPoint as IPEndPoint,
RemoteEndPoint = connectionContext.RemoteEndPoint as IPEndPoint
};
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Middleware/HttpConnectionMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public Task OnConnectionAsync(ConnectionContext connectionContext)
Protocols = _protocols,
ServiceContext = _serviceContext,
ConnectionFeatures = connectionContext.Features,
MemoryPool = memoryPoolFeature.MemoryPool,
MemoryPool = memoryPoolFeature?.MemoryPool ?? System.Buffers.MemoryPool<byte>.Shared,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Ninds Ninds Jan 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

Copy link
Contributor Author

@Ninds Ninds Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Ninds Ninds Jan 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. We don't have an understanding where it does that. I'd like to understand before making assumptions.
  2. 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.

Transport = connectionContext.Transport,
LocalEndPoint = connectionContext.LocalEndPoint as IPEndPoint,
RemoteEndPoint = connectionContext.RemoteEndPoint as IPEndPoint
Expand Down