Yield return deferred iteration problems

Question

I know that yield return takes advantage of lazy loading but I'm wondering if I might be misusing the iterator or quite possibly need a refactor.

My recursive iterator method returns all the ancestors of a given PageNode including the pageNode itself.

public class PageNodeIterator {
    //properties and constructor left out for brevity

    public IEnumerable<IPageNode> ancestorsOf(IPageNode pageNode) {
        if(pageNode == null) throw new ArgumentNullException(("pageNode"));

        if (pageNode.url != pageNodeService.rootUrl) {
            yield return pageNode;
            if (pageNode.parent != null)
                foreach (var node in ancestorsOf(pageNode.parent))
                    yield return node;
        }
    }
}

In my call to ancestorsOf, I'm calling the method and then reversing the order of the returned IEnumerable, but since loading is deferred the call doesn't actually happen until I call ToArray() on the next line and at that point pageNodeService in my iterator method is null and a null reference exception is thrown.

ancestors = pageNodeIterator.ancestorsOf(currentNode).Reverse();
return ancestors.ToArray()[1].parent.children;

So, I'm wondering where I've gone wrong. What would be the proper way to use an iterator in this case, if at all?

I'm also wondering why pageNodeService is null at the time of execution. Even the execution is deferred shouldn't it still hold a value?

Beware (very much) the recursive IEnumerable with yield. It has very surprising and undesirable memory characteristics. http://blogs.msdn.com/b/wesdyer/archive/2007/03/23/all-about-iterators.aspx Consider maintaining your own stack/queue: http://blogs.msdn.com/b/ericlippert/archive/2005/08/01/recursion-part-two-unrolling-a-recursive-function-with-an-explicit-stack.aspx — spender, Jul 23 '13 at 15:12
What in the world happens when you have 2 yield returns in a function?? — Jonesopolis, Jul 23 '13 at 15:12
No problem with 2+ yield returns. State machine will make a stop on more places. — pero, Jul 23 '13 at 15:13
_at that point pageNodeService in my iterator method is null_ - Why? That shouldn't happen. — H H, Jul 23 '13 at 15:13
Crazy. I didn't know that was possible. Seems way more confusing than necessary. — Jonesopolis, Jul 23 '13 at 15:14
@bflemi3: does you method works correctly when you remove iterator stuff ? First make sure you have the algorithm correctly done. Maybe write some unit tests. Then add iterator support. Your code will stay almost the same. — pero, Jul 23 '13 at 15:14
@PetarRepac Haven't tried that yet, but I have other recursive methods that are working just fine. I'm almost positive it has to with deferred loading from `yield return`, just don't know why. — bflemi3, Jul 23 '13 at 15:19
@PetarRepac in the case of this method, the requirement was to return all ancestors including the starting pageNode. Perhaps the naming is a little misleading. — bflemi3, Jul 23 '13 at 15:21
@HenkHolterman I agree, but I don't have a good enough understanding of the .net framework to be able to answer that. It seems fishy though - I'll look into that. — bflemi3, Jul 23 '13 at 15:34
i think maybe its with .children,i guess children are also pagenodes. — terrybozzio, Jul 23 '13 at 16:02
Where does `pageNodeService` come from? How do you set it? Do you ever reset it to `null`? Ideally, could you include a short, but complete code that demonstrates this? — svick, Jul 23 '13 at 17:57

score 10 · Answer 1 · answered Jul 23 '13 at 17:45

I don't know where your bug is, and StackOverflow is not a service for debugging your code; I would solve your problem by running it in the debugger and looking for the bug.

However I will take this opportunity to point out that this:

public IEnumerable<IPageNode> AncestorsOf(IPageNode pageNode) {
    if(pageNode == null) throw new ArgumentNullException(("pageNode"));
    // Do stuff that yields

is slightly problematic because none of the code in the block runs until MoveNext is called for the first time. In other words, if you do:

var seq = AncestorsOf(null); // Not thrown here!
using (var enumtor = seq.GetEnumerator())
{
    bool more = enumtor.MoveNext(); // Exception is thrown here!

which is very surprising to people. Instead write your code like this:

public IEnumerable<IPageNode> AncestorsOf(IPageNode pageNode) {
    if(pageNode == null) throw new ArgumentNullException(("pageNode"));
    return AncestorsOfIterator(pageNode);
}
private IEnumerable<IPageNode> AncestorsOfIterator(IPageNode pageNode)
{
    Debug.Assert(pageNode != null);
    // Do stuff that yields 
}

spender · Accepted Answer · 2013-07-23T16:26:16.620

3

Not really an answer... more of a suggestion for an alternative implementation that eliminates recursion. Too long to post as a comment.

    public IEnumerable<IPageNode> ancestorsOf(IPageNode pageNode) {
        if(pageNode == null) throw new ArgumentNullException(("pageNode"));
        Stack<IPageNode> stack = new Stack<IPageNode>();
        stack.Push(pageNode);
        while(stack.Any())
        {
            IPageNode n=stack.Pop();
            if (n.url != pageNodeService.rootUrl) {
                yield return n;
                if(n.parent != null)
                {
                    stack.Push(n.parent);
                }
            }
        }
    }

Thinking about it, you could remove the Stack altogether:

public IEnumerable<IPageNode> ancestorsOf(IPageNode pageNode) {
    if(pageNode == null) throw new ArgumentNullException(("pageNode"));
    IPageNode n = pageNode;
    while(n != null && n.url != pageNodeService.rootUrl)
    {
        yield return n;
        n = n.parent;
    }
}

edited Jul 23 '13 at 16:26

answered Jul 23 '13 at 15:24

spender

117,338
33
229
351

3

[Here](http://blogs.msdn.com/b/toub/archive/2004/10/29/249858.aspx) is some further reading on why this change is rather important; performance really suffers in recursive iterator blocks; more so than regular recursive functions. – Servy Jul 23 '13 at 15:34
Yes, that's a better explanation that I offered in my comments above. – spender Jul 23 '13 at 15:36
I don't see how this traverses up through all the parents of the given `pageNode` though? – bflemi3 Jul 23 '13 at 15:51
1

@bflemi3: because the parent goes on the stack and is retrieved from the stack on the next iteration of the loop. I had a think about what you're actually doing, and realise that no stack or recursion is really necessary. Take a look at the second code sample and let me know if this still fulfils your intent. – spender Jul 23 '13 at 16:27
This really all boiled down to `pageNodeService` having a `protected set` on it. So when I used `Activator` to create the instance it wasn't able to set that property so it was null. I did however refactor my method with your second solution, so I'm going to give you the answer. Thanks again spender. – bflemi3 Jul 23 '13 at 18:23

score 2 · Answer 3 · edited May 23 '17 at 12:00

2

Does it even make sense to use yield in this place - since by calling Reverse, all the stuff must be buffered anyway so you could instead just return the complete list of ancestors.

edited May 23 '17 at 12:00

Community

1
1

answered Jul 23 '13 at 15:17

Marwie

3,177
3
28
49

pero · Answer 4 · 2013-07-23T15:34:15.327

0

Add starting node outside this iterator if you need it.

public class PageNodeIterator {
    //properties and constructor left out for brevity

    public IEnumerable<IPageNode> ancestorsOf(IPageNode pageNode) {
        if(pageNode == null) throw new ArgumentNullException(("pageNode"));

        if (pageNode.url != pageNodeService.rootUrl)
        {
            if (pageNode.parent != null ) 
            {
                yield return pageNode.parent;
                yield return ancestorsOf(pageNode.parent);
            }
        }
    }
}

edited Jul 23 '13 at 15:34

answered Jul 23 '13 at 15:25

pero

4,169
26
27

The ancestor iteration should continue until `pageNode.url != pageNodeService.rootUrl`. That logic isn't contained in your method. – bflemi3 Jul 23 '13 at 15:28
1

Updated, but then the method should have some other name, because we are nor returning just ancestors, there are other requirements. Maybe better to put these additional requirement outside iterator ? – pero Jul 23 '13 at 15:37
I agree, I'll look into refactoring that. Thanks Petar :) – bflemi3 Jul 23 '13 at 15:39

Yield return deferred iteration problems

4 Answers4

Linked