NullReferenceException hides actual exception

[lauxjpn]

For some MySQL database server implementations (e.g. MariaDB or MySQL < 8.0), unsupported expressions like OUTER APPLY cannot be translated. We throw late in the query pipeline (in MySqlParameterBasedSqlProcessor), because EF Core might still decide to use some unsupported expressions after a RelationalQueryTranslationPostprocessor implementation has been called.

The following exception e.g. is raised by the NorthwindSplitIncludeQueryMySqlTest.Include_collection_with_outer_apply_with_filter test. The exception itself is expected and we expect it to be propagated all the way to the user:

System.InvalidOperationException: The LINQ expression 'OUTER APPLY Projection Mapping:
(
    SELECT TOP(5) o.OrderID
    FROM Orders AS o
    WHERE c.CustomerID == o.CustomerID
    ORDER BY c.CustomerID ASC
) AS t' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to either AsEnumerable(), AsAsyncEnumerable(), ToList(), or ToListAsync(). See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlCompatibilityExpressionVisitor.CheckTranslated(Expression translated, Expression original) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\ExpressionVisitors\Internal\MySqlCompatibilityExpressionVisitor.cs:line 61
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlCompatibilityExpressionVisitor.CheckSupport(Expression expression, Boolean isSupported) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\ExpressionVisitors\Internal\MySqlCompatibilityExpressionVisitor.cs:line 51
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlCompatibilityExpressionVisitor.VisitOuterApply(OuterApplyExpression outerApplyExpression) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\ExpressionVisitors\Internal\MySqlCompatibilityExpressionVisitor.cs:line 42
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlCompatibilityExpressionVisitor.VisitExtension(Expression extensionExpression) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\ExpressionVisitors\Internal\MySqlCompatibilityExpressionVisitor.cs:line 28
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Microsoft.EntityFrameworkCore.Query.SqlExpressions.SelectExpression.VisitChildren(ExpressionVisitor visitor) in E:\Sources\EFCore-5.0\src\EFCore.Relational\Query\SqlExpressions\SelectExpression.cs:line 2878
   at System.Linq.Expressions.ExpressionVisitor.VisitExtension(Expression node)
   at Pomelo.EntityFrameworkCore.MySql.Query.ExpressionVisitors.Internal.MySqlCompatibilityExpressionVisitor.VisitExtension(Expression extensionExpression) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\ExpressionVisitors\Internal\MySqlCompatibilityExpressionVisitor.cs:line 32
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression node)
   at Pomelo.EntityFrameworkCore.MySql.Query.Internal.MySqlParameterBasedSqlProcessor.ProcessSqlNullability(SelectExpression selectExpression, IReadOnlyDictionary`2 parametersValues, Boolean& canCache) in E:\Sources\Pomelo.EFCore.MySql-5.0\src\EFCore.MySql\Query\Internal\MySqlParameterBasedSqlProcessor.cs:line 44
   at Microsoft.EntityFrameworkCore.Query.RelationalParameterBasedSqlProcessor.Optimize(SelectExpression selectExpression, IReadOnlyDictionary`2 parametersValues, Boolean& canCache) in E:\Sources\EFCore-5.0\src\EFCore.Relational\Query\RelationalParameterBasedSqlProcessor.cs:line 64
   at Microsoft.EntityFrameworkCore.Query.Internal.RelationalCommandCache.GetRelationalCommand(IReadOnlyDictionary`2 parameters) in E:\Sources\EFCore-5.0\src\EFCore.Relational\Query\Internal\RelationalCommandCache.cs:line 86
   at Microsoft.EntityFrameworkCore.Query.Internal.SplitQueryingEnumerable`1.Enumerator.InitializeReader(DbContext _, Boolean result) in E:\Sources\EFCore-5.0\src\EFCore.Relational\Query\Internal\SplitQueryingEnumerable.cs:line 198
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteImplementation[TState,TResult](Func`3 operation, Func`3 verifySucceeded, TState state) in E:\Sources\EFCore-5.0\src\EFCore\Storage\ExecutionStrategy.cs:line 173

However, it results in a NullReferenceException for _resultCoordinator.DataReaders here, which is then propagated to the user instead of our actual exception:

efcore/src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs

Line 219 in 551c58a

foreach (var dataReader in _resultCoordinator.DataReaders)

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.EntityFrameworkCore.Query.Internal.SplitQueryingEnumerable`1.Enumerator.Dispose() in E:\Sources\EFCore-5.0\src\EFCore.Relational\Query\Internal\SplitQueryingEnumerable.cs:line 219
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Microsoft.EntityFrameworkCore.TestUtilities.QueryAsserter.AssertQuery[TResult](Func`2 actualQuery, Func`2 expectedQuery, Func`2 elementSorter, Action`2 elementAsserter, Boolean assertOrder, Int32 entryCount, Boolean async, String testMethodName) in E:\Sources\EFCore-5.0\test\EFCore.Specification.Tests\TestUtilities\QueryAsserter.cs:line 103

Include provider and version information

EF Core version: 5.0.0-rc.1

[smitpatel]

Fixed in #23185

@lauxjpn - Let us know if you still hit this error after upgrading to fixed code so I can investigate into it.
Also afaik EF wouldn't change a join type after TranslationPostProcessor. Is this being violated at some point?

[lauxjpn]

Fixed in #23185

Thanks!

Also afaik EF wouldn't change a join type after TranslationPostProcessor. Is this being violated at some point?

I believe so. This seems to be the case for some OUTER APPLY or CROSS APPLY statements. I can investigate later today to find out again, which tests failed for us with a MySqlException instead of the expected translation exception when using MariaDB (that does not have LATERAL support, our translation for those expressions).

[smitpatel]

CollectionJoinApplyingExpressionVisitor should be the last one changing tables. So if base is called beforehand by providers, it should work.

[lauxjpn]

@smitpatel Thanks to bringing my attention back to the issue. As it turns out, we just had a bug in our MySqlCompatibilityExpressionVisitor code:

    public class MySqlCompatibilityExpressionVisitor : ExpressionVisitor
    {
        protected override Expression VisitExtension(Expression extensionExpression)
            => extensionExpression switch
            {
                RowNumberExpression rowNumberExpression => VisitRowNumber(rowNumberExpression),
                CrossApplyExpression crossApplyExpression => VisitCrossApply(crossApplyExpression),
                OuterApplyExpression outerApplyExpression => VisitOuterApply(outerApplyExpression),
                ExceptExpression exceptExpression => VisitExcept(exceptExpression),
                IntersectExpression intersectExpression => VisitIntercept(intersectExpression),
                ShapedQueryExpression shapedQueryExpression => shapedQueryExpression.Update(
                    Visit(shapedQueryExpression.QueryExpression),
                    shapedQueryExpression.ShaperExpression), // <-- missing the Visit() call
                _ => base.VisitExtension(extensionExpression)
            };

        protected virtual Expression VisitCrossApply(CrossApplyExpression crossApplyExpression)
            => CheckSupport(crossApplyExpression, _options.ServerVersion.SupportsCrossApply);
}

This was not an issue when processing the expression visitor at the very end of the pipeline, because ShapedQueryExpressions would not exist anymore.

[Obi-Dann]

Would it be possible to backport that fix for 5.0.x? The change looks quite significant, perhaps it's possible to backport just the part that affects the Dispose method of SplitQueryingEnumerable.Enumerator in, say, 5.0.4. It will be causing quite a lot of pain for us once we finally upgrade to version 5. Happy to submit a PR, if it's ok

[ajcvickers]

@Obi-Dann Can you explain a bit more how the type of exception thrown is impacting you here? (This will help us determine if this issue is something we will patch.)

[freerider7777]

@ajcvickers We just came to the same issue - strange new exceptions appearing in logs do not make production support happy

[freerider7777]

Is it fixed in 5.0.* with Npgsql 5.0.7?

[AndriySvyryd]

@freerider7777 No, it was only fixed in 6.0.0-preview1 and later

[freerider7777]

@AndriySvyryd It's a pity... Very easy to simulate - I connect to office network using VPN. If I disconnect from VPN when running my asp.net core service, I receive this errors in logs which hide the real problem... Is there a workaround?

It's critical for production to understand what is the real problem, please fix it...

[lauxjpn]

It's critical for production to understand what is the real problem, please fix it...

@freerider7777 A quick solution for finding the underlying issue is to implement a FirstChanceException Event handler.

It could log all exceptions (that includes those that are thrown and caught internally). The exception you are looking for should then be logged before the NullReferenceException.

[freerider7777]

@lauxjpn Thanks for that! I think it would double the logs... Maybe I write a new issue, that EF 5.* cannot be used in production with such problems?

[lauxjpn]

I think it would double the logs.

Could be the case, depending on how many internal exceptions are being thrown in your app (in EF Core and providers, it is not common to throw internal exceptions, except when checking for database existence).
Consider using a special log or log level just for this case, so you can easily turn it off again once you found the issue.

Maybe I write a new issue, that EF 5.* cannot be used in production with such problems?

If you feel strongly about it, it might be a good idea. After all, being able to correctly diagnose an issue is an important thing.
If the risk is low and the benefit high, it might make it into a servicing release.
However, the hurdle to jump for servicing releases is pretty high, so the counter argument could be, that users might already have implemented exception handlers targeting the NRE for 5.0.

But the EF Core team will be able to give you a better estimated, what the chances for a 5.0 backport are.

[freerider7777]

@Obi-Dann Do you have a solution for this issue with EF5 (maybe a patch to compile it from sources)?

[Obi-Dann]

@freerider7777 No, not really. We haven't migrated to EF 5 because of performance issues with split queries that require us to rewrite the majority of the queries.
I'm not sure I understand push back on backporting it, the fix is just to add a null check, but perhaps a few places affected that I don't about and that could make it harder.
For us, it's not an issue anymore because we'll be targeting upgrade to EF Core 6

[freerider7777]

@Obi-Dann Thanks... Are you now using 3.1? What performance issues did you have? As far as I understand EF 3.1 uses single queries and you can use it in EF5, why switch to split queries?

[Obi-Dann]

@Obi-Dann Thanks... Are you now using 3.1? What performance issues did you have? As far as I understand EF 3.1 uses single queries and you can use it in EF5, why switch to split queries?

We're on EF 2 still because we have a lot of split queries (that was the default behaviour) and it isn't really a priority atm to rewrite them. But because of #24808 we can't keep using split queries in the same way as they were in ef 2

[freerider7777]

We're also using 2.2, and I've also written a couple of issues on inefficient query changes (more inner queries than 2.2). MS drives us to the idea of using more stable libraries... But we need interceptors, so now we're examining all the produced queries, maybe it's better to write them in sql from the beginning...

[lauxjpn]

MS drives us to the idea of using more stable libraries...

The change from EF Core 2.2 to 3.0 was huge and pretty much the last chance for the EF Core team gain full control over the query pipeline, which they did.
But the other side of the coin is, that this change had a high impact on existing apps, potentially destabilizing them with bad performance in cases where existing queries would cause cartesian explosions.

Since 3.0, EF Core in general and the query pipeline in particular have been very stable when it comes to upgrading, at least that is the case for the applications that I am maintaining, where I did not have to change a thing in regards to query performance. In fact, while I believe that none of my queries got slower, some got faster due to more man hours spent on query optimization by the EF Core team and provider maintainers since 3.0.

So in my experience, once you have made the major jump from 2.2 to 3.0/3.1/5.0/6.0 (which can be daunting), you are unlikely to encounter significant upgrade issues in the future.

But we need interceptors, so now we're examining all the produced queries, maybe it's better to write them in sql from the beginning...

That is a legitimate question. In most projects, I automatically measure at least the time of all major operations (e.g. Web API calls) and the time of all query executions, as part of my application infrastructure (I would do this with any technology stack really, not just when it comes to EF Core).

Then its just a matter of establishing a value of what can be considered as "fast enough" and checking the actual execution time against it.
You then only spend developer time on those few queries that really benefit from some manual optimization (either by adding missing indices or rewriting the query).

---

However, it is also a popular approach to use Dapper for all the read only querying (so your SQL gets executed exactly how you wrote it) and then EF Core for insert/update/delete operations (so you don't have to track those yourself).

[freerider7777]

So in my experience, once you have made the major jump from 2.2 to 3.0/3.1/5.0/6.0 (which can be daunting), you are unlikely to encounter significant upgrade issues in the future.

Hehe... Let us hope for this :) We're with MS technologies for 20+ years, with EF for 10 (like that) years and they always seem "moving ground" :))) EF was created in 2008 and still not stabilized (of course moving to .Net Core took place and so on and so on, but simple joins with "Include" still not in good shape...)
Edmx models are gone, entity sql is gone (which was a rather strange thing), always some renamings (DbQuery suddenly deprecated, needing to rename to DbSet). Same methods with a bit different behavior (AsNoTracking without entity resolution, thanks for AsNoTrackingWithIdentityResolution in EF 5, why not make a "new method" with "new behavior"? ).
(Not to mention that we used Silverlight and Workflow Foundation (4 incompatible with 3.5) which are dead... :)) )

And mostly - it's a pity that split queries (seemed like compatibility with 2.2) are useless for now...

[lauxjpn]

Yeah, there has definitely been some movement in technologies over the year and Microsoft has bit of a track record to burn through certain technologies fast. However, EF Core has its own code base, is build from scratch, and while similar in concept to EF, they are not the same technology.

So my opinion above is really only targeted at EF Core. Once you upgrade to at least 3.0, there are only very few breaking changes and those are usually only relevant to a small user group. The EF Core team is of course implementing new features and improving existing ones in major releases, but has also around 100K tests to ensure, that existing stuff does not break. So the general stability of the EF Core is very high.

As for split queries, they generally work quite nicely and seem to have a broader scope as the did back in 2.2, even though certain cases that worked more efficiently back in 2.2. are now not as efficient anymore as they were back then. Those are the potential cases where, if you encounter performance issues, you either have to split/rewrite the query manually yourself, or automate the process with some code.

There is this old Microsoft saying, that products are only really usable after reaching versions 3. So I guess it was quite clever of the EF team back then, to directly start with EF4. 😄
For EF Core, I think this still holds true. Version 3.x is the first one I would consider mature.

[freerider7777]

products are only really usable after reaching versions 3

This should be stated with big letters on first pages of the docs :) (BTW first EF was 3.5 which was unusable...)