Skip to content
This repository was archived by the owner on Dec 24, 2022. It is now read-only.

Fix stack overflow error when serializing complex object graph#331

Closed
dotnetpowered wants to merge 2 commits into
ServiceStack:masterfrom
dotnetpowered:master
Closed

Fix stack overflow error when serializing complex object graph#331
dotnetpowered wants to merge 2 commits into
ServiceStack:masterfrom
dotnetpowered:master

Conversation

@dotnetpowered

Copy link
Copy Markdown

Track visited objects during serialization to prevent stack overflows. Prevents serializer from getting stuck in endless loops by not revisiting objects. Related to scriptcs bug: scriptcs/scriptcs#255

@mythz

mythz commented May 20, 2013

Copy link
Copy Markdown
Member

Hi Brian,

The static VisitorCache is not threadsafe. Could you upload a failing test I can look at so I can have a look at an optimal way to solve this. I think it's better to have an explicit depth (as mentioned in the thread) would be better here.

@dotnetpowered

Copy link
Copy Markdown
Author

Hi, The VisistorCache was built to solve the thread safety issue by using the textwriter as the key. However, I didn't go far enough. I've added locking which should solve the problem. I think the explicit depth is a good idea too, but I think it still makes sense to have protection for looping regardless of the depth option. This requires no code change to existing code using the library to gain this protection.

Here is the code that fails with a stackoverflow:

class Program
{
    class person
    {
        public string name { get; set; }
        public person teacher { get; set; }
    }
    static void Main(string[] args)
    {
        person j1 = new person();
        j1.teacher = new person { name = "sam", teacher = j1 };
        j1.name = "bob";
        Console.WriteLine(j1.ToJsv());
        Console.Read();
    }
}

@mythz mythz closed this in dfa69b9 May 24, 2013
@mythz

mythz commented May 24, 2013

Copy link
Copy Markdown
Member

Hi Brian,

I ended up going with a configurable JsConfig.MaxDepth feature instead, since this commit broke a few tests and we only try to use lockless synchronization to avoid contention with locks and maintaining a cache of every visited object has more computational and GC overhead.

The JsConfig.MaxDepth can be set within a scope block so it doesn't affect other threads, e.g:

class person
{
    public string name { get; set; }
    public person teacher { get; set; }
}

using (JsConfig.With(maxDepth:4))
{
    var p = new person();
    p.teacher = new person { name = "sam", teacher = p };
    p.name = "bob";
    p.ToJsv().Print();  
    p.ToJson().Print(); //also works with JSON
    p.PrintDump();  //uses p.ToJsv() under the hood
}

I've included your test above as part of the test suite. This will be available in the next version of ServiceStack v3.9.47 that I'll try to deploy this weekend.

thx for reporting.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants