10

Recently my co-worker showed me a block of code that was not working correctly:

public class SomeClass
{
    private IList<Category> _categories;

    public void SetCategories()
    {
        _categories = GetCategories() ?? new List<Category>();
        DoSomethingElse();
    }

    public IList<Category> GetCategories()
    {
        return RetrieveCategories().Select(Something).ToList();
    }
}

(I am aware that the operator is superfluous since the linq ToList will always return a list, but that is how the code was set up).

The problem was that _categories was null. In the debugger, setting a breakpoint on _categories = GetCategories() ?? new List<Category>(), then stepping over to DoSomethingElse(), _categories would still be null.

Directly setting _categories to GetCategories() worked fine. Splitting the ?? in to a full if statement worked fine. The null coalescing operator did not.

It is an ASP.NET application, so a different thread could possibly be interfering, but it was on his machine, with only him connected in a browser. _cateogories is not static, or anything.

What I'm wondering is, how could this possibly happen?

Edit:

Just to add to the bizarreness, _categories is never set anywhere besides that function (apart from initializing the class).

The exact code is like so:

public class CategoryListControl
{
    private ICategoryRepository _repo;
    private IList<Category> _categories;

    public override string Render(/* args */)
    {
        _repo = ServiceLocator.Get<ICategoryRepository>();
        Category category = _repo.FindByUrl(url);
        _categories = _repo.GetChildren(category) ?? new List<Category>();
        Render(/* Some other rendering stuff */);
    }
}

public class CategoryRepository : ICategoryRepository
{
    private static IList<Category> _categories;

    public IList<Category> GetChildren(Category parent)
    {
        return _categories.Where(c => c.Parent == parent).ToList<Category>();
    }
}

Even it GetChildren magically returned null, CategoryListControl._categories still should never, ever be null. GetChildren should also never, ever return null because of IEnumerable.ToList().

Edit 2:

Trying out @smartcaveman's code, I found out this:

Category category = _repo.FindByUrl(url);

_categories = _repo.GetChildren(category) ?? new List<Category>();

_skins = skin; // When the debugger is here, _categories is null

Renderer.Render(output, _skins.Content, WriteContent); // When the debugger is here, _categories is fine.

As well, when testing if(_categories == null) throw new Exception(), _categories was null on the if statement, then stepping over the exception was not thrown.

So, it seems like this is a debugger bug.

Snea
  • 1,867
  • 2
  • 14
  • 21
  • Is it because you are instantiating in the statement? Possibly fine but I haven't used it this way before. – PMC Feb 01 '11 at 20:10
  • Could uou check again? I'm thinking "debug error". – H H Feb 01 '11 at 20:15
  • 2
    The fascinating part here is that `GetCategories()` is unable to return `null` anyway. The coalescing operator is of no use anyway. – Pieter van Ginkel Feb 01 '11 at 20:20
  • Just found [this topic](http://stackoverflow.com/questions/4619593/is-the-null-coalesce-operator-thread-safe). – Marlon Feb 01 '11 at 20:22
  • @Marlon, the strange thing is that I do not think _categories is at any point explicitly set to null, besides when SomeClass gets initialized. – Snea Feb 01 '11 at 20:26
  • 2
    @Pieter: Yeah, he knows. – BoltClock Feb 01 '11 at 20:27
  • Are you sure you are getting that concrete implementation when you call the ServiceLocator? Just checking because it's one of those things one easily misses sometimes (like trying to set breakpoints in Release mode :)) – Skurmedel Feb 01 '11 at 20:59
  • @Skurmedel, we were able to step in to it in the debugger, so I believe so. – Snea Feb 01 '11 at 21:03
  • @Snea: Ok, that's a bit unfortunate. – Skurmedel Feb 01 '11 at 21:06
  • Given this was over 2 years ago, I don't suppose you can remember, but were you by any chance running the code using the ReSharper unit test runner? I've just had a very similar situation, again with the null coalescing operator and even to the point where I was able to fix the code then break it again. After I quit from VS to post on StackOverflow, after that I was unable to reproduce the issue. See http://stackoverflow.com/questions/19352130/why-doesnt-the-null-coalescing-operator-work-in-this-situation – Tim Long Oct 14 '13 at 02:16

6 Answers6

2

This could be a problem with the debugger rather than with the code. Trying printing out the value or doing a null check after the statement with the coalesce operator.

Tim H
  • 36
  • 1
  • I believe the program was throwing a NullReferenceException in the first place, which lead him to attaching the debugger and seeing that problem. I suppose it is still possible, though. – Snea Feb 01 '11 at 20:18
  • I guess I was mistaken that a NullReferenceException was being thrown. A check for null will return that the value is not null, even though when the debugger was directly on the if statement, it showed _categories being null. – Snea Feb 02 '11 at 19:22
2

The null-coalescing operator is not broken. I use it in a similar manner all the time quite successfully. Something else is going on.

Greg D
  • 43,259
  • 14
  • 84
  • 117
1

If you sure that its because of threading issue, then use the lock keyword. I believe this should work.

public class SomeClass
{
    private IList<Category> _categories;

    public void SetCategories()
    {
        lock(this) 
        {
          _categories = GetCategories() ?? new List<Category>();
          DoSomethingElse();
        }
    }

    public IList<Category> GetCategories()
    {
        return RetrieveCategories().Select(Something).ToList();
    }
}
AbrahamJP
  • 3,425
  • 7
  • 30
  • 39
1

Try doing a clean build. Build menu-> clean, then debug again. The code itself is fine.

Chris Shain
  • 50,833
  • 6
  • 93
  • 125
  • 1
    Hmmmm. Could this be because of stale binaries? +1 – Pieter van Ginkel Feb 01 '11 at 20:28
  • It's possible, but we did change the code to a manual if statement, which worked, then back, and it stopped working again. Wouldn't stale binaries also make the debugger complain about the source being different? I'll see if we can reproduce it again today. – Snea Feb 01 '11 at 20:38
  • Then it isn't stale binaries. I assume the code you posted is a simplification- is there anything else that we should see? – Chris Shain Feb 01 '11 at 20:42
  • @Christ Shain, edited with the actual code - but I still don't see how it could affect `_categories` being null. – Snea Feb 01 '11 at 20:53
  • Try this... Make _categories a Property, and in the setter, do a console.writeline or some other logging to find out when it is being set, on what thread, etc. If you are debugging correctly, the only possible answer is that another thread is updating its value. – Chris Shain Feb 01 '11 at 21:12
1

(1) DoSomethingElse() could be setting the _categories field to null, before the error appears. A way to test this is to make the _categories field readonly. If that is the error, then you will get a compiler error that a readonly field cannot be used as an assignment target.
(2) Your _categories field is being set via some other function in a different thread. Either way, the following should fix your problem, or at least make it clear where it is.

public class SomeClass
{
    private static readonly object CategoryListLock = new object();
    private readonly List<Category> _categories = new List<Category>();
    private bool _loaded = false;

    public void SetCategories()
    {
        if(!_loaded)
        {
            lock(CategoryListLock)
            {
                if(!_loaded)
                {
                    _categories.AddRange(GetCategories());
                    _loaded = true;
                }
            }
        }
        DoSomethingElse();
    }

    public IList<Category> GetCategories()
    {
        return RetrieveCategories().Select(Something).ToList();
    }
}

**After seeing your edit, it looks like you have two different fields that are IList<Category> _categories. It doesn't make sense that the _categories field in the CategoryListControl is null, but the static _categories in the CategoryRepository class looks like it should be null based on what you posted. Perhaps you are getting confused about which field is throwing the error. I understand that the line is called in the CategoryListControl, so your error will say it is in the CategoryListControl class, but the actual exception could be from the GetChildren() method, which attempts to make a children list from a null list). Since the fields are named the same thing, it is easy to see how they could get confused. Test this by making the _categories field in CategoryRepository a readonly initialized field.

Even if the _categories field in the CategoryRepository is not always null, it could be subject to any of the threading concerns that I explained how to fix for the Control class**

ItTo make sure you are debugging the correct _categories field, try this.

    _categories = GetCategories() ?? new List<Category>();
    if(_categories == null){
          throw new Exception("WTF???");
     }
    DoSomethingElse();

If you don't get the Exception with "WTF???" then you know the source of the error is elsewhere.

And, regarding the Linq extensions: Neither Where() nor ToList() can return null. Both methods will throw an ArgumentNullException if any parameters are null. I checked this with reflector.

Please let us know what results you get with this. I'm curious now too.

smartcaveman
  • 41,281
  • 29
  • 127
  • 212
  • Unfortunately, `_categories` is _only_ set from that function. – Snea Feb 01 '11 at 20:37
  • @smartcaveman, I am pretty sure that it is the _categories in CategoryListControl, and that an exception is being thrown because of that. In the debugger, we can step over the null coalescing operator line without an exception, and on the very next line CategoryListControl._categories is null. If CategoryRepository._cateogries was null, then the null coaslescing operator line would throw an exception on Where(), because the input can't be null. Either way, the ToList in GetCategories cannot return null, can it? – Snea Feb 02 '11 at 18:05
  • @smartcaveman, Check the edit. Trying out your code seems to indicate a debugger problem. I've accepted Tim H's answer because it is technically what I believe the problem is, but I would accept yours too if I could. Thanks. – Snea Feb 02 '11 at 19:24
  • Visual Studio 2010 attached to w3wp.exe (iis7) – Snea Feb 02 '11 at 20:18
1

This could happen because you have optimizations turned on - in that case the assignment may be delayed for as long as the compiler can demonstrate that doing so doesn't change the result. Of course, this looks weird in the debugger, but it's perfectly OK.

Eamon Nerbonne
  • 47,023
  • 20
  • 101
  • 166