Skip to content

Named Singletons sub Singletons - Tenancy issue #228

Closed
Closed
Named Singletons sub Singletons - Tenancy issue#228

Description

@stephenlautier
stephenlautier
opened on Jul 12, 2019

We had hit an issue when using multi-tenancy (see sample below), when being a singleton.

Basically, we have something like the following small sample:

  • Service A - singleton per tenant (IHeroDataClient) - 2 different implementations per tenant
    • Service B - singleton per tenant (HerozClientSub) - same implementation but instance per tenant
    • Service C - singleton for all (IDataClientManager) - same instance for all tenants (shared)

In the above example if we remove Service B (HerozClientSub) it works, so basically when having nested singleton deps it doesn't resolve the tenant.

By changing Service A and Service B to scoped it will also work, but it's not ideal as it will create instances redundantly, and also not sure why it doesn't resolve or if it's intended or a bug.

This is the sample if you want to look at it:

Changes to cause the issue: sketch7/dotnet.multitenancy@feature/initial-imp...issue/nested-singleton-tenant-bug
Some interest points

Basically, the implementation of how tenancy works is how you had suggested.

Am I doing something wrong? should this work?

Thanks

Activity

  1. ipjohnson commented on Jul 12, 2019

    @ipjohnson
    ipjohnson
    on Jul 12, 2019
    Owner

    Sorry you ran into that let me take a look this weekend.

  2. stephenlautier commented on Jul 12, 2019

    @stephenlautier
    stephenlautier
    on Jul 12, 2019
    Author

    No problem, thanks for your help as always

  3. ipjohnson commented on Jul 16, 2019

    @ipjohnson
    ipjohnson
    on Jul 16, 2019
    Owner

    So I took a look and I'm unclear when you say it doesn't work. What specifically is the error you are getting.

    I will say that Singleton as a lifestyle is restrictive on what it can depend on and will not carry or use the injection context from the call (it starts fresh, no injection context).

  4. stephenlautier commented on Jul 16, 2019

    @stephenlautier
    stephenlautier
    on Jul 16, 2019
    Author

    So by starting from fresh, I guess I'm using it wrong/unsupported. What we would like to achieve is to have singletons, and singleton dependencies per tenant as following:

    Tenant A - IHeroDataClient (A) singleton -> HerozClientSub (A) singleton
    Tenant B - IHeroDataClient (B) singleton -> HerozClientSub (B) singleton

    Perhaps the ideal is to look at the sample

  5. ipjohnson commented on Jul 16, 2019

    @ipjohnson
    ipjohnson
    on Jul 16, 2019
    Owner

    It sounds very much like you need a custom lifestyle SingletonPerTenant. I don’t have a solution for you right at this moment but can you use SingletonPerScope for the next week while I come up with something?

    Ultimately I think this is a use case worth supporting I’ve just never had someone ask for it.

  6. stephenlautier commented on Jul 16, 2019

    @stephenlautier
    stephenlautier
    on Jul 16, 2019
    Author

    Yes, we are not blocked by this we found a way around it by using less DI and create a manual factory. Not the ideal but at least it's working. It would be much easier if it will be supported as a consumer would need to know about these quirks and there are certain scenarios unhandled.

  7. ipjohnson commented on Jul 31, 2019

    @ipjohnson
    ipjohnson
    on Jul 31, 2019
    Owner

    Sorry been swamped, I haven't forgotten about this. I'm hoping this weekend to finish the SingletonPerKey lifestyle.

  8. stephenlautier commented on Jul 31, 2019

    @stephenlautier
    stephenlautier
    on Jul 31, 2019
    Author

    @ipjohnson I understand, thanks!

  9. ipjohnson
    added a commit that references this issue on Aug 11, 2019

    adding SingletonPerKey lifestyle for issue #228

    6ef0ee4

  10. ipjohnson commented on Aug 11, 2019

    @ipjohnson
    ipjohnson
    on Aug 11, 2019
    Owner

    I've released a new version with SingletonPerKey lifestlye. Let me know how it works

  11. ipjohnson commented on Aug 11, 2019

    @ipjohnson
    ipjohnson
    on Aug 11, 2019
    Owner

    You can see an example unit test here

  12. stephenlautier commented on Aug 12, 2019

    @stephenlautier
    stephenlautier
    on Aug 12, 2019
    Author

    Great, I will try to give it a shot.

    In which version is it? 7.1.0-beta768?

  13. ipjohnson commented on Aug 12, 2019

    @ipjohnson
    ipjohnson
    on Aug 12, 2019
    Owner

    Correct.

  14. stephenlautier commented on Aug 12, 2019

    @stephenlautier
    stephenlautier
    on Aug 12, 2019
    Author

    I just tried it out quickly and it seems to be working fine, altho I had to do a change in the usage from your test, I used scope instead of context if i use the context i wont get the tenant

    return configuration.Lifestyle.SingletonPerKey((scope, context) =>
{
	var tenant = scope.GetTenantContext();
	return tenant?.Key;
});

    image

  15. ipjohnson commented on Aug 12, 2019

    @ipjohnson
    ipjohnson
    on Aug 12, 2019
    Owner

    That makes sense. The one change I would make is to provide a key if one doesn't exist otherwise it's going to throw an exception.

  16. stephenlautier commented on Aug 12, 2019

    @stephenlautier
    stephenlautier
    on Aug 12, 2019
    Author

    Great, thanks for your help and for this great feature 👍

  17. ipjohnson commented on Aug 13, 2019

    @ipjohnson
    ipjohnson
    on Aug 13, 2019
    Owner

    I’m going to mark this as closed. If you run into any issue let’s open a new ticket to track the changes

  18. ipjohnson
    closed this as completedon Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions