Streamlining feature cache and object re-initialiation code paths #516
Conversation
Shouldn't increase object size, but does improve maintainability
|
/cc @benaadams |
|
/cc @DamianEdwards also Outside of pooling this also has the benefit of making DefaultHttpContext more useful as a base class. By overriding the Initialize methods an app author can subclass the request/response/etc companion objects, otherwise you have no control over those associated objects. |
| { | ||
| } | ||
|
|
||
| protected override HttpRequest InitializeHttpRequest() |
There was a problem hiding this comment.
You need to override all of methods in order to properly pool right?
There was a problem hiding this comment.
That's a policy choice, really - the ConnectionInfo/WebSocket objects might be used infrequently enough it could be could be more efficient to let them be garbage collected. But yes, you would override all child object management methods to take full control
Adding comment about reason for public field
| @@ -274,6 +271,7 @@ private static void TestFeatureProperties(object value, IFeatureCollection featu | |||
| { | |||
| if (property.Name.Contains("Feature")) | |||
| { | |||
| Console.WriteLine($"{value.GetType().Name} {property.Name}"); | |||
|
Looks good to me |
. 
| { | ||
| var type = value.GetType(); | ||
|
|
||
| var fields = type |
There was a problem hiding this comment.
This doesn't verify anything anymore because the object layout has changed. The only field it will find is FeatureReferences<FeatureInterfaces> _features;.
There was a problem hiding this comment.
Unless it manages to unroll the nested structs?
There was a problem hiding this comment.
Yeah, a unit test that reflects over the private implementation details seemed a little 
to me in the first place, but comparing the nested struct to default(T) could certainly be done...
There was a problem hiding this comment.
Test updated to assert _features field state is reset to default(T). Though I'm still highly suspicious of the practice.
|
shrug |
|
What's the emoji for shrug? 😐? 😶? 🙈? :) |
|
|
| TestCachedFeaturesAreSet(context.WebSockets, features); | ||
| } | ||
|
|
||
| void TestCachedFeaturesAreSet(object value, IFeatureCollection features) |
There was a problem hiding this comment.
TestCachedFeaturesAreSet needs to be updated as well.
There was a problem hiding this comment.
That one is already getting the property, and getting the interface type, and asserting they are the same. Isn't using reflection on fields to verify a private has been changed highly unusual? I can add unit test over FeatureReferences is that verify it's calling through only once, but doing the testing of that class indirectly seems wrong.
There was a problem hiding this comment.
I was trying to test if the cached (private var) was correct for the current and any future additions so act as a regression safety test; even found a few instances where it wasn't actually being saved to the private var with the test. (So the cache wasn't caching - though it publicly worked)
|
LGTM, and does look a lot better |
|
|
|
Hmm, related #522 though not a blocker; thought I could work round this issue for Frame but the call stack runs through here. Current flow |
| Collection.Set(feature); | ||
| if (!cleared) | ||
| { | ||
| Cache = default(TCache); |
There was a problem hiding this comment.
Bit late; but not sure about this if statement? What's it doing?
Shouldn't increase object size, but does improve maintainability