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

Ability to get/know the type of object for which the factory was called #76

Closed
Neakita opened this issue Dec 13, 2024 · 13 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@Neakita
Copy link

Neakita commented Dec 13, 2024

Greetings!

I have a situation where I want to get an ILogger (from Serilog) dependency in a certain class:

internal sealed class WriteableBitmapPool : IDisposable
{
	...
	public WriteableBitmapPool(ILogger logger)
	{
		_logger = logger;
	}
	...
}

Ideally, the default factory should provide a contextual logger, i.e. it should be obtained from the Log.ForContext<T>() or Log.ForContext(typeof(T)) method, so when WriteableBitmapPool is being created it should get logger instance from result of calling Log.ForContext<WriteableBitmapPool>() method.

Perhaps the API should look something like

private void Setup() => DI.Setup(nameof(Composition))
		...
		.Bind<ILogger>().To(context => Log.ForContext<TSomething>())
		...

where TSomething is replaced with something more suitable

so that the generator can convert this to something like
Serilog.ILogger logger = Log.ForContext<WriteableBitmapPool>();
just before WriteableBitmapPool instance creation

Alternatively add some Type property to IContext so factory could look like
.Bind<ILogger>().To(context => Log.ForContext(context.ObjType))

@NikolayPianikov
Copy link
Member

The problem is that one object can be used as a dependency to create objects of different types. This depends, for example, on the lifetime. I made an example of how I would implement logging.

@Neakita
Copy link
Author

Neakita commented Dec 16, 2024

The problem is that one object can be used as a dependency to create objects of different types. This depends, for example, on the lifetime.

I agree that this is a problem with an ambiguous solution, but in my opinion it can be solved by having each specific ILogger<T> have its own lifetime, i.e. if we register a singleton ILogger<T> that can be obtained by requesting ILogger in the constructor, all objects of the Dependency type will receive their single instance of Logger<Dependency>, and all objects of the Service type will receive their single instance of Logger<Service>. I believe this is how generic Dependency<TT> work.

I made an example of how I would implement logging.

The solution with creating additional types representing loggers for specific types certainly has a place to be, but it creates unnecessary verbosity and problems in cases where, for example, it is necessary to pass a non-default logger.

Also, apparently, in this example there is a composition configuration error (it breaks off in mid-sentence)

@NikolayPianikov NikolayPianikov self-assigned this Dec 16, 2024
@NikolayPianikov NikolayPianikov added the enhancement New feature or request label Dec 16, 2024
@NikolayPianikov
Copy link
Member

NikolayPianikov commented Dec 16, 2024

@Neakita could you please review this approach?

OwnerType is the owner type of the instance being created:

  • For the Transient instance, this is the owner type.
  • For instances of Singleton and Scoped, this is the composition type.
  • For the PerBlock instance, this is the type that generated the block.
  • For a PerResolve instance, it is the composition root type.

@Neakita
Copy link
Author

Neakita commented Dec 17, 2024

@NikolayPianikov

This is perfect for my case, however, the choice of name and the fact that the meaning depends on lifetime can be confusing, although I can't think of something more appropriate.

Unless, judging by the name, the dependency is given to the object in ownership, which I would consider a bad approach, because then it should take responsibility for managing the lifetime of its dependencies, which it can only do if it creates them itself, although in this case it gets them from outside and the owner is considered a Composition that should manage their lifetime, there is an inconsistency here when the lifetime is, for example, transient.

@NikolayPianikov
Copy link
Member

NikolayPianikov commented Dec 17, 2024

Unless, judging by the name, the dependency is given to the object in ownership, which I would consider a bad approach, because then it should take responsibility for managing the lifetime of its dependencies, which it can only do if it creates them itself, although in this case it gets them from outside and the owner is considered a Composition that should manage their lifetime, there is an inconsistency here when the lifetime is, for example, transient.

@Neakita Yes that's a reasonable point. Please see this option.

@Neakita
Copy link
Author

Neakita commented Dec 17, 2024

@NikolayPianikov

As far as I understand, in this example you replaced OwnerType with ConsumerTypes, where in the case of Singleton or PerResolve dependency there will be several types that use it.

It seems to me that the previous example providing the OwnerType property was more understandable and easier to use, whereas the option with ConsumerTypes looks more flexible, I can't think of ways to use it at the moment. However, it seems to me that the Consumer looks more appropriate naming than the Owner.

My point was that, as you said in your report on DotNext, Pure.DI is intentionally not responsible for the lifetime of dependencies of the Transient type implementing IDisposable because it requires a lot of overhead.

I have a suggestion about this that I would like to make separate issue later.

In my opinion, something like ConsumerType will work more than adequately, however, for such a scenario, open generics can also be adapted for grouping by type, so Log.ForContext<TT> can be a singleton for each concrete TT, although I suppose it is too difficult to implement for such a narrow use case, but it will also worsen performance because it will be necessary to use lock for all these resolves, additional time will be spent on compiling all generic methods, and objects will not be able to be collected by GC until Composition is disposed.

I'm sorry if I'm confusing or distracting you.

@NikolayPianikov
Copy link
Member

As far as I understand, in this example you replaced OwnerType with ConsumerTypes, where in the case of Singleton or PerResolve dependency there will be several types that use it.

Yes, that's the way it is

It seems to me that the previous example providing the OwnerType property was more understandable and easier to use, whereas the option with ConsumerTypes looks more flexible, I can't think of ways to use it at the moment. However, it seems to me that the Consumer looks more appropriate naming than the Owner.

The array option would be useful when we want to account for all consumers.

My point was that, as you said in your report on DotNext, Pure.DI is intentionally not responsible for the lifetime of dependencies of the Transient type implementing IDisposable because it requires a lot of overhead.

Right now it is possible to control transient IDisposable. See this or this example.

In my opinion, something like ConsumerType will work more than adequately, however, for such a scenario, open generics can also be adapted for grouping by type, so Log.ForContext<TT> can be a singleton for each concrete TT, although I suppose it is too difficult to implement for such a narrow use case, but it will also worsen performance because it will be necessary to use lock for all these resolves, additional time will be spent on compiling all generic methods, and objects will not be able to be collected by GC until Composition is disposed.

I haven't figured out how to use context to call a method with a generic type.

@Neakita
Copy link
Author

Neakita commented Dec 17, 2024

I haven't figured out how to use context to call a method with a generic type.

Not a context, but marker type such as TT, TT1, etc, some brand new TConsumer.
But again, if OwnerType/ConsumerTypes/ConsumerType will be implemented, in my case I would prefer to use it because Log.ForContext<T>() under the hood still calls Log.ForContext(typeof(T)), however, a certain TConsumer marker type can also be potentially useful.

@Neakita
Copy link
Author

Neakita commented Dec 17, 2024

The array option would be useful when we want to account for all consumers.

Not a logger case, maybe something other. I saw the generated code and it can actually be stackalloc ReadOnlySpan which prevents unnecessary array allocation.

@NikolayPianikov
Copy link
Member

Not a logger case, maybe something other. I saw the generated code and it can actually be stackalloc ReadOnlySpan which prevents unnecessary array allocation.

This is already implemented for some cases, e.g. see here.

@Neakita
Copy link
Author

Neakita commented Dec 17, 2024

This is already implemented for some cases, e.g. see here.

I'm talking about this.
upd: Actually i'm wrong, it's not possible to create Span/ReadOnlySpan with ref types.
Instead it's possible to use ArrayPool to prevent allocations, but it's an implementation details.

@NikolayPianikov
Copy link
Member

Not a context, but marker type such as TT, TT1, etc, some brand new TConsumer. But again, if OwnerType/ConsumerTypes/ConsumerType will be implemented, in my case I would prefer to use it because Log.ForContext<T>() under the hood still calls Log.ForContext(typeof(T)), however, a certain TConsumer marker type can also be potentially useful.

Yeah I get your idea, created a ticket and will think about it.

NikolayPianikov added a commit that referenced this issue Dec 17, 2024
@NikolayPianikov
Copy link
Member

Fixed in 2.1.42

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

No branches or pull requests

2 participants