Find the bugWhy do I get a Null Reference Exception?
Originally posted at 4/12/2011
Can you spot the NRE hiding in the code?
if (parentObject!= null) { lock (parentObject) { if (parentObject != null) { properties = properties.Clone(); parentObject[parentKey] = this; parentObject = null; } } }
More posts in "Find the bug" series:
- (29 Feb 2016) When you can't rely on your own identity
- (05 Jan 2016) The case of the degrading system–Answer
- (04 Jan 2016) The case of the degrading system
- (11 Sep 2015) The concurrent memory buster
- (20 Apr 2011) Why do I get a Null Reference Exception?
- (25 Nov 2010) A broken tree
- (13 Aug 2010) RavenDB HiLo implementation
- (25 Jul 2010) Accidental code reviews
Comments
other thread assigned null to parentObject while this thread is executing right after the second if clause but before the first statement
Nullifying the parent object prevents from entering this code again. Is this your NRE?:>
If parentObjectCopy is null you should get an ArgumentNullException, so my vote goes to properties being null.
.NET may fail when trying to
lock (parentObjectCopy)
and that is null. You probably wanted to lock on parentObject instead.either:
properties is null
or
parentKey is null
Why would you do this:
properties = properties.Clone();
What purpose does it serve?
Fredrik,
You are correct, I published part of the solution to this, opps. Fixed now
It depends on what kind of variables parentObject & parentObjectCopy are. If parentObjectCopy is not shared between threads ( private static ) then the lock is kind of useless since other threads will not block on it and can set partenObject to null while other threads try to call the indexer.
Looks like you have a race condition between checking the parentObject for null and then attempting to lock it.
As mjkirkham says, there's a race condition.
When a context switch happens right after the first if statement, but before the lock, another thread could execute "parentObject = null;". When the 'first' thread then resumes control, it will try to lock on a parentObject which is now null.
Okay, I was wrong. The (now removed) parentObjectCopy being null would have caused an ArgumentNullException, as Frederik said.
Iain: parentKey being null should cause an ArgumentNullException, too.
In that case, setting properties (let's assume it's a instance variable) to properties.Clone() does seem suspicious ...
You are setting the lock object to null.
When the lock context ends, Monitor.Exit(parentObject) will be called and will throw.
But I thought it threw ArgumentNullException as well
I've run the code and Monitor.Exit does not throw an exception. I'm guessing that lock creates a temp reference object.
I wish we had a bit more context to this (i/e scope of variables etc.)
Its no NRE, but other ArgumentNullException would be if a second entrant attempted to lock ParentObject after it was set to null.
2nd Entrant -> parentObject != null, continue
1st entrant -> parentobject = null
2nd Entrant -> lock(parentObject), throws
Setting the lock to null seems like a bad idea.
The LOCK statement is converted into two methods:
==============
Monitor.Enter(object o)
//code
Monitor.Exit(object o)
==============
In the Exit method the locking object has been set to null, and as described in the documentation of the method an ArgumentNullException would be thrown.
Actually, on .NET 4.0, lock (obj) translates into try { Monitor.Enter(local = obj, ref lockTaken); ... } finally { if (lockTaken) Monitor.Exit(local); }. This means that the problem is not the Exit method; the Enter method is the one throwing NRE.
Monitor.Enter will throw an ArgumentNullException...Not sure about NullReferenceException.
Oh yeah, you're right. Don't know about the NullReferenceException then.
even in 4.0 Enter throws an ArgumentNullException. if obj is null.
Well, since all accesses to "parentObject" are safe, and Monitor.Enter throws ArgumentNullException, then I guess "properties" must be null.
One more thing: generally the framework doesn't allow NRE to bubble up. it's usually something more meaningful. It has to be in his code.
The compiler is optimizing the output with reordering:
Compiler Reordering:
This can be prevented if a barrier is used and/or making the object volatile to avoid reodering.
That's not it. There's a dataflow dependence so the compiler would never make that optimization. Also, on the CLR, all stores have release semantics, meaning there's an implicit barrier that does not allow stores to be reordered with other stores.
@Bek Yep. My initial mix up stemmed from a bit too much java programming, where there's no equivalent to ArgumentNullException: when validating parameters, you throw a NullPointerException.
The object used for a lock is being set to null inside the lock.
I think "properties" is not initialized, so when calling properties.clone() , you get a NRE ..?
If some other piece of code sets parentObject to a different (non-null) value, then the parentObject you lock on might not be the same parentObject you use a few lines down. This might cause a NullReferenceException (and other strange behavior).
This is far-fetched, since the snippet doesn't show anyone setting parentObject, but still... Locking an object that can be replaced (by null or whatever) doesn't seem like a good idea to me.
properties is null
or
parentKey is null
This line is wierd
properties = properties.Clone();
lock() is not safe against reentrant code on the same thread. So maybe the implementation of properties.Clone() recurses to the block of code in question (and does not infinitely recurse, since we didn't get a stack overflow; suppose it has some sort of recursion limit). It will re-enter the lock, Clone() once more, null out the parentObject (I'm assuming it's not a local variable on the stack, since you're locking on it), then unwind to find parentObject null. Boom.
How to install rhino mocks? the usability of this page is very bad. where can i get binaries. i dont want to install git or something like that. this is to freaky. it should be easier to use at all.
User24, And this has what to do with this post?
If i would have spend less time in looking how to get rhino mocks installed, i would have more time for things like this (finding NULL References).
Kind Regards && sorry for wrong Post ;)
Comment preview