Wanna Review My Code?
Posted by Davy Brion on February 23rd, 2010
I just wrote some code, and i’d like your opinion on it. The thing is, i’m not going to provide any context as to what it does or why certain decisions were made since i know you guys like to be challenged. You also might want to keep the following in mind when reading it:
- it might contain bugs that i’m not aware of
- it contains weird parts that were either brainfarts on my part, things i did on purpose to avoid possible issues, or both
- it contains at least one bug that i know about, yet don’t care about
- i removed the comments that i originally put in it to make things more interesting
- i haven’t tested the code yet
- i haven’t even executed it yet
- i think it’s ok… but i’m not sure
Questions will be answered if you have them (and i’m sure you will) though i can’t make any promises as to how soon i can answer them… I will post a follow-up post to discuss the code in its entirety later on, though i’ll probably wait a few days to do so.
This is the code:
public class TenantSessionFactoryManager : ITenantSessionFactoryManager
{
private readonly ITenantContext tenantContext;
private readonly ITenantInfoHolder tenantInfoHolder;
private readonly string mappingAssemblyName;
private readonly object writeLock = new object();
private Dictionary<Guid, ISessionFactory> sessionFactories;
public TenantSessionFactoryManager(ITenantContext tenantContext, ITenantInfoHolder tenantInfoHolder, string mappingAssemblyName)
{
this.tenantContext = tenantContext;
this.tenantInfoHolder = tenantInfoHolder;
this.mappingAssemblyName = mappingAssemblyName;
sessionFactories = new Dictionary<Guid, ISessionFactory>();
}
public ISession CreateSessionForCurrentTenant()
{
var tenantId = tenantContext.CurrentTenantId;
if (!sessionFactories.ContainsKey(tenantId))
{
CreateSessionFactoryForCurrentTenant();
}
return sessionFactories[tenantId].OpenSession();
}
private void CreateSessionFactoryForCurrentTenant()
{
lock (writeLock)
{
var tenantId = tenantContext.CurrentTenantId;
if (!sessionFactories.ContainsKey(tenantId))
{
var connectionString = tenantInfoHolder.GetDatabaseConnectionString(tenantId);
var sessionFactory = new Configuration()
.Configure()
.AddProperties(new Dictionary<string, string>
{
{ "connection.connection_string", connectionString },
{ "cache.region_prefix", "Tenant_" + tenantId }
})
.AddAssembly(mappingAssemblyName)
.BuildSessionFactory();
var newDictionary = new Dictionary<Guid, ISessionFactory>(sessionFactories);
newDictionary[tenantId] = sessionFactory;
sessionFactories = newDictionary;
}
}
}
public void RemoveSessionFactoryForTenant(Guid tenantId)
{
if (!sessionFactories.ContainsKey(tenantId))
{
return;
}
lock (writeLock)
{
if (!sessionFactories.ContainsKey(tenantId))
{
return;
}
var sessionFactory = sessionFactories[tenantId];
var newDictionary = new Dictionary<Guid, ISessionFactory>(sessionFactories);
newDictionary.Remove(tenantId);
sessionFactories = newDictionary;
sessionFactory.Dispose();
}
}
}
February 23rd, 2010 at 5:24 pm
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?
February 23rd, 2010 at 5:54 pm
Why do you replace the sessionFactories dictionary with a new copy every time you need to add or remove a new SessionFactory?
February 23rd, 2010 at 6:28 pm
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…
February 23rd, 2010 at 6:50 pm
Not that I think the it would happen in reality…
February 23rd, 2010 at 6:50 pm
@Akward,
He repeats the check inside of the lock also…
February 23rd, 2010 at 8:33 pm
@Stefan – that’s not the problem…
February 23rd, 2010 at 9:19 pm
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.
February 24th, 2010 at 9:59 am
@Ken
if i use the SyncRoot, i have to use it for each access to the dictionary
@JeroenH
it’s a copy-on-write implementation… reading from the dictionary is thread-safe, as long as no other thread mutates the state of the dictionary. when a ISessionFactory needs to be added to or removed from the dictionary, i create a new one, add the new sessionfactory to it or remove the old one from it, and then replace the field that holds the dictionary. during this entire operation, any other thread can still safely perform the read since the field that is used by the thread always references a dictionary that is never mutated.
@Awkward Coder
sessions aren’t stored in the dictionary… they’re sessionfactories. i don’t _think_ there’s a race condition between creating a new one, and removing one from the dictionary but i definitly could be wrong… creating and removing is both done within a lock on the same object, so i think it’s safe… if i’m wrong there, i’d really appreciate you pointing out what i missed
@Pho
you are correct about the bug
normally, it shouldn’t occur because by the time we need to remove a tenant, the users of that tenant shouldn’t even have access to the application anymore. and even if they did still have access, they’re no longer a paying customer (hence the removal of the tenant) so any exception they run into isn’t really something i’d be worried about
February 24th, 2010 at 12:21 pm
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
February 24th, 2010 at 12:23 pm
@Awkward Coder
aha, that is indeed technically possible
February 24th, 2010 at 12:57 pm
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;
}
}
February 24th, 2010 at 1:05 pm
@Awkward
i think you got something mixed up there because that either returns an ISession or an ISessionFactory
February 24th, 2010 at 1:31 pm
I blame notepad
February 25th, 2010 at 6:20 am
I will use IDictionary and will not use {} for single IF statement.
February 25th, 2010 at 8:30 pm
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.
February 26th, 2010 at 8:26 am
@Adam
true… but it also falls under the ” they’re no longer a paying customer so i no longer care about them” category