-
Notifications
You must be signed in to change notification settings - Fork 308
Add ObjectPool<StringBuilder>
to default services
#652
Conversation
- can now use pooled `StringBuilder`s to address aspnet/HttpAbstractions#561 and similar issues nit: Fix incomplete test noted when adding new one
} | ||
|
||
[Fact] | ||
public async Task ObjectPoolOfStringBuilderRegistered_EvenWhenStartupCtorThrows() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually care about ObjectPool in the StartupThrows scenario. You can find a simpler test to copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions? I don't see much that just validates services are registered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Configure(app => { })
would be an adequate replacement for UseStartup in this test.
Do not merge this until we see how aspnet/HttpAbstractions#561 looks. |
🆙📅 and aspnet/HttpAbstractions#587 is out. |
@davidfowl Gag check? |
This is odd. I'm not sure this makes sense. Hosting isn't a dumping ground for arbitrary services like this.... |
@davidfowl - what do you propose? Use a static pool? That might make the most sense if we're talking about HttpAbstractions/WebUtilities |
Yes, I was going to suggest a static pool for something like this with optional overloads to pass in a pool into whatever method is there (low level method). |
Let's discuss this offline. We already have at least one other pattern for |
Will move instantiation of the pool to Kestrel as decided offline. |
StringBuilder
s to addressResponseCookies.Append()
allocates lots, big lots HttpAbstractions#561 and similar issuesnit: Fix incomplete test noted when adding new one