Add tenant reconfiguration; update locking. #8

marcrocny · 2017-08-15T20:32:53Z

Remove() allows for reconfiguration of an existing tenant; fixes #5.

Changes the locking to use a reader-writer lock.

…nt()` for management/reconfiguration of tenants.

tillig · 2017-08-16T16:08:25Z

I really like this! Thanks!

Thinking about reconfiguring a tenant and multi-threaded apps. I imagine reconfiguring a tenant on the fly may be like this:

mtc.RemoveTenant("tenant1");
mtc.ConfigureTenant("tenant1", b => b.RegisterType<T>());

Hypothetically, between RemoveTenant and ConfigureTenant something on another thread could resolve something from the tenant1 tenant, which would cause the tenant scope to be re-created, which would cause ConfigureTenant to throw.

I can think of a couple of options to get around this:

Expose the lock (or a lock access mechanism) for consumers. This would allow a large set of atomic operations to be executed by a consumer. Of course, that's not a great API and could create some even more challenging locking issues within the MTC class.
Change ConfigureTenant so it doesn't throw on a second call; instead have it drop any existing tenant scope and create a new one as needed. Not a bad idea, but could be confusing to people trying to "append to a tenant" by calling ConfigureTenant a bunch of times for the same tenant and not realizing that you can't do that.
Add a ReconfigureTenant override that handles the remove/add as an atomic operation internally. RemoveTenant is still interesting, but ReconfigureTenant would be a remove/add all at once and handle the locking to ensure atomicity.

What do you think? Is there a fourth option? Or maybe I'm off the deep end, which isn't a far stretch since my current work project has driven me batty. 😆

marcrocny · 2017-08-16T17:30:59Z

Thanks very much.

Admittedly, I should have given the atomic-reconfiguration case more thought. Was focused more on the concern of supporting an "admin" side of an application where clients are added or removed as a single operation, not often reconfigured during their lifetime.

(For that requirement I have used a less "drastic" mechanism: load all the known implementations of an interface and use ResolveKeyed() or ResolveNamed(). Although, dynamically-loaded extensions or plug-ins would be another matter. Haven't had to go there yet.)

Maybe it's just me, but the indeterminate nature of ConfigureTenant() in option 2 has a "smell" to me. Overloading the current functionality doesn't seem necessary--if you're embarking on reconfiguring tenants, a new API seems appropriate.

Option 3, ReconfigureTenant(), I like. Working on that right now...

Option 1, exposing the lock, only makes sense if there are a multitude of other unforeseeable interactions with the tenant-dictionary, or if we really just want this to be as open as possible. We already have a "start," an "end," and--with the addition of ReconfigureTenant()--a "knife-edge cutover". Those three seem to cover all cases, I'm not sure there's a need for the complication. Plus, locking is hard. Even with good docs, exposing the "slim" lock will require the user to reference the code to avoid lock-recursion.

I can't think of any other options either. 🦇, maybe, but pretty cogent 😄

…ve-configure fail-case demo; additional caveats in XML docs.

marcrocny · 2017-08-16T19:27:44Z

Because it proceeds destructively, should it be ReconfigureTenant() or RebuildTenant()?

tillig · 2017-08-16T19:29:11Z

Good question. I could see either way, though I lean toward Reconfigure since that both implies you're changing it and it has the benefit of consistent grammar (configure/reconfigure).

marcrocny · 2017-08-16T19:31:07Z

I'll leave as is, with the appropriate caveats in the XML-docs.

tillig · 2017-08-16T19:31:35Z