28

I am trying to throw and catch an AggregateException. I did not use exceptions very much on C#, but the behaviour I found is a little bit surprising.

My code is:

var numbers = Enumerable.Range(0, 20);

try
{
    var parallelResult = numbers.AsParallel()
        .Where(i => IsEven(i));
    parallelResult.ForAll(e => Console.WriteLine(e));

}
catch (AggregateException e)
{
    Console.WriteLine("There was {0} exceptions", e.InnerExceptions.Count());
}

It is calling the function IsEven

private static bool IsEven(int i)
{
    if (i % 10 == 0)
        throw new AggregateException("i");
    return i % 2 == 0;
}

That throws the AggregateException.

I would expect the code to write every even number in the 0,20 range and "There was 1 exceptions" twice.

What I get is some numbers printed (they are random cause of ForAll) and then the exception is thrown, but not catched and the programs stop.

Am i missing something?

MaPi
  • 1,601
  • 3
  • 31
  • 64
  • 2
    Am not sure why this happens, try changing the `throw new AggregateException("i");` to `throw new ArgumentException("i");` produces the expected result – Sriram Sakthivel Oct 09 '13 at 08:13
  • Throwing `throw new AggregateException("i", new[] { new ArgumentException("i") });` also helps, but no idea why your version crashes the app – Sriram Sakthivel Oct 09 '13 at 08:36
  • 6
    You are confusing the system by throwing the wrong exception. Throw an Argument or InvalidOperationException instead. – H H Oct 09 '13 at 08:58
  • 1
    @SriramSakthivel - _why your version crashes the app_ - I have to speculate, but its InnerExceptions will be empty. Unexpected. – H H Oct 09 '13 at 09:00
  • @HenkHolterman Did you tried? It is strange that App crashes even when we wrap the code with try/catch :( I agree with you `InnerExceptions` should have exceptions, when I do this `throw new AggregateException("i", new[] { new ArgumentException("i") })` it works perfectly – Sriram Sakthivel Oct 09 '13 at 09:38
  • 1
    @SriramSakthivel - but inner code should not throw Aggregate. Not their job. – H H Oct 09 '13 at 09:42
  • 1
    @HenkHolterman Of course, i know it is a bad idea and AggregateException's purpose is to combine many exceptions and preserve StackTrace. but I can't find any documentation which states we shoudn't do that – Sriram Sakthivel Oct 09 '13 at 09:48
  • The inner exception is actually null, but even replacing the exception throw with throw new AggregateException("i", new[] { new ArgumentException("i") }); I get an error saying AggregateException was unhandled by user code – MaPi Oct 09 '13 at 12:50
  • Stop throwing AggregateException – H H Oct 10 '13 at 09:23
  • 1
    The line "parallelResult.ForAll(e => Console.WriteLine(e));" throws a DisposedException "The query enumerator has been disposed" ...strange... – Manuel Amstutz Oct 15 '13 at 10:24

2 Answers2

26

This is actually kind of interesting. I think the problem is that you're using AggregateException in an unexpected way, which is causing an error inside the PLINQ code.

The entire point of AggregateException is to group together multiple exceptions that may occur simultaneously (or nearly so) in a parallel process. So AggregateException is expected to have at least one inner exception. But you're throwing new AggregateException("i"), which has no inner exceptions. The PLINQ code tries to examine the InnerExceptions property, hits some sort of error (probably a NullPointerException) and then it seems to go into a loop of some sort. This is arguably a bug in PLINQ, since you're using a valid constructor for AggregateException, even if it is an unusual one.

As pointed out elsewhere, throwing ArgumentException would be more semantically correct. But you can get the behavior you're looking for by throwing a correctly-constructed AggregateException, for example by changing the IsEven function to something like this:

private static bool IsEven(int i)
{
    if (i % 10 == 0){
        //This is still weird
        //You shouldn't do this. Just throw the ArgumentException.
        throw new AggregateException(new ArgumentException("I hate multiples of 10"));
    }
    return i % 2 == 0;
}

I think the moral of the story is to not throw AggregateException unless you really know exactly what you're doing, particularly if you're already inside a parallel or Task-based operation of some kind.

Jonny Cundall
  • 2,552
  • 1
  • 21
  • 33
Brian Reischl
  • 7,216
  • 2
  • 35
  • 46
  • Please edit so that you have the proper version of code (throwing ArgumentException instead of throwing AggregateException). You did say in the end but I am afraid people who would take a glance and think the posted code is the proper one (which is not). – X.J Oct 15 '13 at 22:21
  • 1
    Threw in a comment that you shouldn't do as I did. – Brian Reischl Oct 15 '13 at 22:24
8

I agree with others: this is a bug in .Net and you should report it.

The cause is in the method QueryEnd() in the internal class QueryTaskGroupState. Its decompiled (and slightly modified for clarity) code looks like this:

try
{
  this.m_rootTask.Wait();
}
catch (AggregateException ex)
{
  AggregateException aggregateException = ex.Flatten();
  bool cacellation = true;
  for (int i = 0; i < aggregateException.InnerExceptions.Count; ++i)
  {
    var canceledException =
        aggregateException.InnerExceptions[i] as OperationCanceledException;
    if (IsCancellation(canceledException))
    {
      cacellation = false;
      break;
    }
  }
  if (!cacellation)
    throw aggregateException;
}
finally
{
  this.m_rootTask.Dispose();
}
if (!this.m_cancellationState.MergedCancellationToken.IsCancellationRequested)
  return;
if (!this.m_cancellationState.TopLevelDisposedFlag.Value)
  CancellationState.ThrowWithStandardMessageIfCanceled(
    this.m_cancellationState.ExternalCancellationToken);
if (!userInitiatedDispose)
  throw new ObjectDisposedException(
    "enumerator", "The query enumerator has been disposed.");

Basically, what this does is:

  • rethrow the flattened AggregateException if it contains any non-cancellation exceptions
  • throw new cancellation exception if cancellation was requested (or return without throwing, I don't really understand that part, but I don't think it's relevant here)
  • else throw ObjectDisposedException for some reason (assuming userInitiatedDispose is false, which it is)

So, if you throw an AggregateException with no inner exceptions, ex will be an AggregateException containing your empty AggregateExcaption. Calling Flatten() will turn that into just an empty AggreateException, which means it doesn't contain any non-cancellation exception, so the first part of the code thinks this is cancellation and doesn't throw.

But the second part of the code realizes this isn't cancellation, so it throws a completely bogus exception.

svick
  • 236,525
  • 50
  • 385
  • 514