Why does this cause a StackOverFlow error?

Question

This is causing a StackOverFlow error and I have an idea why, but I would like a little more detail on why and is my solution for it the way it should be handled. Ok, first things first, the following code causes a StackOverFlow error when I try to assign a value to the property:

private List<Albums> albums
{
    get
    {
        if (Session["albums"] != null)
            return (List<Albums>)Session["albums"];
        else
            return AlbumCollection.GetAlbums();
    }
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

To resolve the above, I changed the name of the property and added another variable to hold the value of the property which resolved the StackOverFlow issue:

private List<Albums> albums = null;
private List<Albums> Albums
{
    get
    {
        if (Session["albums"] != null)
            return (List<Albums>)Session["albums"];
        else
            return AlbumCollection.GetAlbums();
    }
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

Also, am I doing the setter correct, assigning the value and then assigning the Session["albums"] the value in albums? Could I have just done, Session["albums"] = value instead?

Answer 1

You were reassigning to the property itself.

In your case, you are only using the Session.

So this should be fine

private List<Albums> albums
{
    get
    {
     if (Session["albums"] == null)
      Session["albums"] = AlbumCollection.GetAlbums();
     return (List<Albums>)Session["albums"];
    }
    set
    {
     Session["albums"] = value;
    }
}

Answer 2

The problem is with this line:

albums = value;

You are recursively setting the property to value which will call the setter again and again, until it stackoverflows. There's no point in that line of code. Just get rid of it.

I guess there's a false misconception that a property needs to be bound to a field or something. It's not. A property, by itself, is just a couple unrelated methods, that are not required to have any specific relation to each other or to a field. When you retrieve the property value, you just call its get method and use the return value and when you set its value, you call its set method with the appropriate value argument. You don't need to somehow "change" the property value in setter. The semantics are automatically enforced as you are changing the value that get is going to return so the next time you call get, it will return Session["..."] which you have already changed.

Answer 3

Your setter is calling itself recursively in the first example. Your second fixes this.

Yes, you could have done.

Answer 4

Because in your setter, you are calling ... the setter, which goes to the setter, and calls ... the setter ... and ...

 set
 {    
    albums = value;       // < --- This line calls itself again.. 
    Session["albums"] = albums;
 }

What you need to do is just use the the Session["albums"] as the persistent storage for the value... You don't need a private field - that's just creating a redundant copy of the value. Eliminate it entirely, and just put...

private List<Albums> Albums
{    
    get    
    {        
         if (Session["albums"] != null)
              return (List<Albums>) Session["albums"];        
         else            
            return (Session["albums"] = AlbumCollection.GetAlbums());    
    }    
    set    
    {    
        Session["albums"] = value;    
    }
}

In certain scenarios where you do not have a persistent store, it's perfectly acceptable for a public property to have just a private member backing field.

For more info on C# properties in general, check out the MSDN tutorial.

Answer 5

This line is causing your issue, because it winds up calling your getter recursively in an infinite loop:

albums = value;

Answer 6

I do not know the answer about this question but you are obviously on the right website !

Answer 7

You are correct, in the first example you are recursively calling the album setter infinite times, hence the stack overflow. (C# Properties and methods should always start with an uppercase letter btw).

In the second example you could simply use:

Session["albums"] = value;

if you wanted to.

Answer 8

You are correct, the issue is when ever you use "albums" in the first code block you are referring to this setter/getter. Thus when you do albums = value in the setter you are recursively calling the setter again.

Internally, the compiler converts accessors to functions, and it may help you to see your error by doing this yourself:

private List<Albums> albums
{
    set
    {
    albums = value;
    Session["albums"] = albums;
    }
}

When compiled, becomes:

private void set_albums(List<Albums> value)
{
    set_albums(value);
    Session["albums"] = albums;
}

Answer 9

After some brisk searching, this should help you out: http://stackoverflow.com/questions/1929141/why-does-this-cause-a-stackoverflow-error

Answer 10

It's a common mistake to call Session["Albums"] twice. Session indexing is a relatively expensive operation involving a dictionary lookup.

private List<Albums> Albums {    
get
{        
     object stored = Session["albums"];
     if (stored != null)
          return (List<Albums>) stored;
     else
     {      
        var newValue = AlbumCollection.GetAlbums();
        Albums = newValue;
        return newValue;
     }
}    
set    
{    
    Session["albums"] = value;
}}