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

Use ephemeral service info blocks for any lookups before the registry is built. #1220

Merged
merged 6 commits into from
Nov 4, 2020

Conversation

alistairjevans
Copy link
Member

This change constructs a temporary set of ServiceRegistrationInfo for calls to GetInitializedServiceInfo that take place during the build callbacks that populate the DefaultRegisteredServicesTracker.

Marginal extra performance cost to IfNotRegistered calls at container build time to allocate a new set, and the overhead of evaluating sources multiple times, but once the tracker is marked as Complete, the only extra cost is a single boolean flag check the first time we initialize a ServiceRegistrationInfo. No concurrency issues expected, because the Container Build process is always a single-threaded operation anyway.

Fixes #1217. Also added a test for conditional registrations based on dynamic sources, and on service middleware.

Any suggestions of additional test cases would be most welcome.

… 'OnlyIf' type callbacks will consume ephemeral service information.
@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #1220 into develop will increase coverage by 0.21%.
The diff coverage is 78.26%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1220      +/-   ##
===========================================
+ Coverage    75.89%   76.10%   +0.21%     
===========================================
  Files          189      189              
  Lines         4683     4721      +38     
  Branches       957      969      +12     
===========================================
+ Hits          3554     3593      +39     
+ Misses         674      669       -5     
- Partials       455      459       +4     
Impacted Files Coverage Δ
...ration/ScopeRestrictedRegisteredServicesTracker.cs 80.00% <ø> (ø)
...tofac/Core/Registration/ServiceRegistrationInfo.cs 78.94% <46.66%> (-1.26%) ⬇️
...e/Registration/DefaultRegisteredServicesTracker.cs 83.84% <93.33%> (+4.60%) ⬆️
...ofac/Core/Registration/ComponentRegistryBuilder.cs 79.66% <100.00%> (+0.35%) ⬆️
src/Autofac/Util/AsyncReleaseAction.cs 71.42% <0.00%> (-14.29%) ⬇️
src/Autofac/RegistrationExtensions.Conditional.cs 81.81% <0.00%> (+9.09%) ⬆️
...Autofac/ServiceMiddlewareRegistrationExtensions.cs 33.33% <0.00%> (+13.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bda20f9...0881165. Read the comment docs.

@tillig
Copy link
Member

tillig commented Oct 26, 2020

If the purpose is to resolve #1217 and related things, brainstorming test cases:

  • Un-skip the test for #1217
  • Test combinations in different orders of things that run build callbacks:
    • Registering a module does a callback to run module.Configure - does IfNotRegistered outside the module work with a decorator registered inside a module (and vice versa)?
    • RegisterServiceMiddlewareSource uses a callback for stuff like decorating open generics. Can we (still?) register an open generic outside a module and a decorator inside a module and have it work (and vice versa)?
    • Decorate a component that's IStartable. (Maybe not the actual IStartable interface, but some other interface on the component.)

This seems like some sort of combinatoric data driven test could be done but I'm not sure how that'd work. Probably too many indirections to lambdas and things to make it intelligible long term.

@alistairjevans
Copy link
Member Author

Thanks @tillig; I'll probably just create the additional tests directly. As you say, the setup for them is going to be a bit too varied.

@alistairjevans
Copy link
Member Author

Added more tests based on your comments.

We already have a StartableTypesCanBeDecorated test in DecoratorTests, so I didn't add another one.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

Minor test thing, otherwise I think this is a good solution to the problem. I can't think of a way to deal with it other than this and I've been sort of churning it around in my mind for a few days.

Comment on lines 101 to 102
builder.RegisterGeneric(typeof(MultiServiceGeneric<>)).As(typeof(IService<>)).As(typeof(IService2<>));
builder.RegisterGeneric(typeof(MultiServiceGeneric<>)).As(typeof(IService<>)).IfNotRegistered(typeof(IService2<int>));
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be more clear if two different open generics were being registered. Regardless of whether IfNotRegistered works, IService<> will be a MultiServiceGeneric<>.

@tillig
Copy link
Member

tillig commented Nov 3, 2020

I sometimes wonder if the fluent callback mechanism for registration is worth the trouble. IServiceCollection seems so much easier to deal with in the general case.

@alistairjevans
Copy link
Member Author

I can see some of the benefit of the IServiceCollection approach, but it's worth noting that each individual method for adding services to the collection in MS DI has little or nothing in the way of additional configuration options for that registration. Not sure how well that model would continue to hold up as features are added.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

🔏

@tillig tillig merged commit e9f7a80 into autofac:develop Nov 4, 2020
@tillig
Copy link
Member

tillig commented Nov 4, 2020

I don't think we should change to collection-based registration necessarily, just sometimes the callbacks and the callbacks that call callbacks and the order of those callbacks seems... pretty complex for what actually ends up happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decorator + IfNotRegistered on the decorated type breaks in 6.0
2 participants