microsoft / Trill

Trill is a single-node query processor for temporal or streaming data.
MIT License
1.25k stars 132 forks source link

Exception of type 'System.StackOverflowException' was thrown. #122

Closed JonathanKeav closed 4 years ago

JonathanKeav commented 4 years ago

Hi

I've been experimenting with trill and have come across an issue that looks like a bug. If i run the following code I get a stack overflow error.

SensorReading[] historicData = new[]
       {
                new SensorReading { Time = 1, Value = 0 },
                new SensorReading { Time = 2, Value = 20 },
                new SensorReading { Time = 3, Value = 15 },
                new SensorReading { Time = 4, Value = 30 },
                new SensorReading { Time = 5, Value = 45 }, 
                new SensorReading { Time = 6, Value = 50 },
                new SensorReading { Time = 7, Value = 30 }, 
                new SensorReading { Time = 8, Value = 35 },
                new SensorReading { Time = 9, Value = 60 }, 
                new SensorReading { Time = 10, Value = 20 }
            };

            var streamable1 = historicData
                .ToObservable().ToTemporalStreamable(r => r.Time, r => r.Time + 1);

            var output = streamable1.TumblingWindowLifetime(3)
                   .Aggregate(
                   w => w.Count(),
                   w => w.Average(v => v.Value),
                   (count, avg) => new { Count = count, Avg = avg });

            Console.WriteLine("Output =");
            output.ToStreamEventObservable().ForEachAsync(e => Console.WriteLine(e)).Wait();

It might be that I'm using the linq incorrectly but I've tried various changes and it looks like if i want a tumbling window and also an aggregate with a lambda, like average, max or min, i get this error.

Thanks, Jonathan

peterfreiling commented 4 years ago

Hi @JonathanKeav , I'm not seeing any exception when running this on the latest master with a dummy definition of SensorReading. What is the stack trace of the exception?

JonathanKeav commented 4 years ago

Hi Peter

I'll give you a brief overview of what i have done so far. I am using the trill samples solution which comes with version 2019.5.1.1 added as a nuget package. I added my own project to this trill samples solution with the code supplied in the original issue above and i see a stack overflow error. So today i updated the trill samples solution to use the latest nuget package, version 2019.9.25.1, but i still have the same error.

I then created a .net core 3.0 console app, downloaded the latest master from here, built and referenced that and i am still getting the same error when i run a linq query like this

var output = streamable1.TumblingWindowLifetime(3)
.Aggregate(
                   w => w.Count(),
                   w => w.Average(v => v.Value),
                   (count, avg) => new { Count = count, Avg = avg });

Below is a screenshot of the error which occurs in StreamMessagePool class in StreamMessagePoolColumnar.cs.

image

The error details that appear in the screenshot are all that there are. Selecting "View Details" does not provide any further information.

Thanks, Jonathan

peterfreiling commented 4 years ago

StackOverflowException usually means there are too many nested function calls, so a screenshot of the exception text does not really help. Can you please provide the stack trace itself to see what all of thee nested function calls are?

AlgorithmsAreCool commented 4 years ago

Hey @peterfreiling2 , I was able to reproduce this with his code sample.

Data is here https://gist.github.com/AlgorithmsAreCool/68654a12ac7b9d2af49ac052985d556d

AlgorithmsAreCool commented 4 years ago

@peterfreiling2

https://github.com/microsoft/Trill/blob/4d8b4f8daa4f2ca67316139914e3aaa98cc44bf9/Sources/Core/Microsoft.StreamProcessing/Operators/SnapshotWindow/Tumbling/SnapshotWindowTumblingPipeSimple.cs#L198-L202

🤔🤔🤔

Edit, This call should be to DisposeStateLocal() instead, right?

peterfreiling commented 4 years ago

@AlgorithmsAreCool thanks! Yes, looks like a typo. The repro requirements missing from the original code sample was that we needed row-based execution, and an aggregate state that implements IDisposable.

peterfreiling commented 4 years ago

@AlgorithmsAreCool or @JonathanKeav , would either of you be able to check in the fix? As @AlgorithmsAreCool mentioned, the recursive call to DisposeState should instead be a call to DisposeStateLocal

AlgorithmsAreCool commented 4 years ago

@peterfreiling2 Made a PR #123

peterfreiling commented 4 years ago

Thanks @AlgorithmsAreCool and @JonathanKeav !