You are doing it REALLY wrong, the shortest code review ever

time to read 2 min | 394 words

We got a question from a customer that included the following code:


This was quite unrelated to the question being asked, but it was enough to trigger all sorts of alarms. Just based on this two lines, and the fact that we can assume that _session is a RavenDB session, we can tell quite a lot about the codebase.

To start with, it is broken, probably badly so.

Let us try to break down why. The fact that you are locking on the session means that you access it concurrently. And that is utterly forbidden. The session is not meant to be used concurrently, and Strange Stuff will happen if you try to.

Now, the session doesn’t have thread affinity, which means that if you are calling it serially, you can certainly move it between threads. In fact, with async sessions, that happens quite often. So one would expect that locking on the session would provide sufficient protection for the session from concurrent use.

However, nothing could be further from the truth. Look at the output of this method. It returns User instances, which are used outside the lock. However, if two threads will call this method concurrently, they will each run in turn, taking and releasing the lock as required. Which is great, but it is very likely that they will get the same User instance back (remember, the session implements Identity Map and ensure that within the scope of the session, the same document is always represented by the same object instance). That is when things start going boom, boom, BOOM!

Even if you are locking on the session, you aren’t locking on your entities, and that means that you have concurrent access to them. I haven’t actually reviewed the entity in question, but I’m 99.999999% certain that no attempt was made to make them safe for multi threading and 100% certain that it there was such an attempt, it would fail horribly in some manner quite easily.

Entities are not meant for concurrent usage, and even trying to reason about them effectively is going to be near impossible.

A session is meant to be a scope of work, in fact, look up the Unit of Work pattern, which is exactly how you are expected to use the session.