Wanna Review My Code?

just a couple of points:
1. in RemoveSessionFactoryForTenant() you do a “reverse if” – return if the post-condition is ok, otherwise do the work, while in CreateSessionFactoryForCurrentTenant() you go the other way around, which adds an annoying nesting level. I’d reverse the if on CreateSessionFactoryForCurrentTenant() also
2. I’d also move the first Contains check into CreateSessionFactoryForCurrentTenant(), for two reasons:
i) will make the main method clearer
ii) will make it clear that we follow the two-phase check
3. question: why use an external lock object, and not the .SyncLock of the dictionary?

Isn’t there a race condition between creating & removing a session?

You could be in the middle of assigning the new session into the dictionary (which you replace everytime) and you request the (new) session be removed but it won’t be cos you’re checking outside of the lock in the remove…

@Akward,

He repeats the check inside of the lock also…

my refactorings based on 2 minutes of apathically staring at this code:
1. rename method CreateSessionFactoryForCurrentTenant() to EnsureSessionFactoryForCurrentTenantExists() because you’re only conditionally creating a new session factory.
2. remove the contains check in CreateSessionForCurrentTenant(). if you did this to avoid writelocks as much as possible, move the check to the method that does the actual lock. in both cases –> remove the temp variable in the public method
3. (this one is on the extremely subjective side and purely to improve readability) i’m not really a fan of implicitly adding new key value pairs and prefer using the Add(K,V) method, especially if contexts like this where you know you are adding and are certain a keyvaluepair for a given key does not yet exist.

you probably have your reasons why this isn’t already the case i guess

as for the known bug, i guess theoretically it could be that between CreateSessionFactoryForCurrentTenant() and the actual dictionary lookup, the key value pair for the current tenantid is already removed? won’t happen though.

if one thread is in the middle of executing CreateSessionFactoryForCurrentTenant – inside the lock and hasn’t yet updated the dictionary and another thread executes RemoveSessionFactoryForTenant for the same tenant id then the first if statement outside of the lock statement will return true and exit the method, whilst the other thread completes and updates the dictionary with the new SessionFactory.

Chances of this actually happening are zero though.

or I could be complete wrong, it’s been known

plus I would change the CreateSessionForCurrentTenant to remove the other race condition…

private ISession CreateSessionFactoryForCurrentTenant()
{
lock (writeLock)
{
var tenantId = tenantContext.CurrentTenantId;
if (sessionFactories.ContainsKey(tenantId))
{
return sessionFactories[tenantId].OpenSession();
}

var connectionString = tenantInfoHolder.GetDatabaseConnectionString(tenantId);
var sessionFactory = new Configuration()
.Configure()
.AddProperties(new Dictionary
{

{ \connection.connection_string\, connectionString },

{ \cache.region_prefix\, \Tenant_\ + tenantId }

})
.AddAssembly(mappingAssemblyName)
.BuildSessionFactory();
var newDictionary = new Dictionary(sessionFactories);
newDictionary[tenantId] = sessionFactory;
sessionFactories = newDictionary;

return sessionFactory;
}
}

I blame notepad

If CreateSessionForCurrentTenant and RemoveSessionFactoryForTenant execute at the same time with an unreasonable amount of bad luck it’s possible that Dispose gets called on the sessionfactory right before OpenSession.
I don’t see how that can be avoided without using the same lock for reading the dictionary as well.