Wrap 'open DataReader' exception with something that says to enable MARS

[kirthik]

DECISION: We will wrap the exception in something that says to use MARS.

If MARS is not enabled in connection string, browser shows the following error. Error message should suggest user to enable MARS to get past the error

An unhandled exception occurred while processing the request. InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first. Remotion.Linq.Clauses.ResultOperatorBase.InvokeExecuteMethod(MethodInfo method, Object input) DataStoreException: An error occured while running a data store operation. See InnerException for details. Microsoft.Data.Entity.Query.EntityQueryExecutor.ExecuteCollection[T](QueryModel queryModel)

[kirthik]

@anpete mentioned that enabling MARS is a requirement now for project k apps (that need DB) so a lot of users will hit this issue.Guiding users in the right direction I think will be very useful here.

[rowanmiller]

@anpete - is required for all queries now, or just certain scenarios?

[anpete]

@rowanmiller Should just be certain queries.

@kirthik Were you running an explicit query? If so, what was it?

[kirthik]

@anpete - I thought you said " It is currently a limitation that we always require MARS". Did I misunderstand?

My scenario - I published music store to azure and created sql azure database. Connection string that is set does have not MARS by default. So on checkout of album in music store app, I saw this error in browser

[anpete]

@kirthik Sorry, I should have said "often require MARS". Are you able to post the stack trace?

[kirthik]

InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first. Remotion.Linq.Clauses.ResultOperatorBase.InvokeExecuteMethod(MethodInfo method, Object input) Remotion.Linq.Clauses.ResultOperatorBase.InvokeGenericExecuteMethod[TInput,TResult](IStreamedData input, Func2 genericExecuteCaller)
Remotion.Linq.Clauses.ResultOperators.ValueFromSequenceResultOperatorBase.ExecuteInMemory(IStreamedData input)
Microsoft.Data.Entity.Query.ResultOperatorHandler.ExecuteResultOperator[TSource,TResult](IEnumerable1 source, ResultOperatorBase resultOperator, StreamedSequenceInfo streamedSequenceInfo) lambda_method(Closure , QueryContext , QuerySourceScope ) Microsoft.Data.Entity.Query.EntityQueryModelVisitor.<>c__DisplayClass01.b__1(QueryContext qc)
Microsoft.Data.Entity.Relational.RelationalDataStore.Query[TResult](QueryModel queryModel, StateManager stateManager)
Microsoft.Data.Entity.Query.EntityQueryExecutor.ExecuteCollection[T](QueryModel queryModel)
DataStoreException: An error occured while running a data store operation. See InnerException for details.
Microsoft.Data.Entity.Query.EntityQueryExecutor.ExecuteCollection[T](QueryModel queryModel)
Microsoft.Data.Entity.Query.EntityQueryExecutor.ExecuteSingle[T](QueryModel queryModel, Boolean _)
Remotion.Linq.Clauses.StreamedData.StreamedSingleValueInfo.ExecuteSingleQueryModel[T](QueryModel queryModel, IQueryExecutor executor)
Remotion.Linq.Clauses.StreamedData.StreamedSingleValueInfo.ExecuteQueryModel(QueryModel queryModel, IQueryExecutor executor)
Remotion.Linq.QueryModel.Execute(IQueryExecutor executor)
Remotion.Linq.QueryProviderBase.Execute(Expression expression)
Remotion.Linq.QueryProviderBase.System.Linq.IQueryProvider.Execute[TResult](Expression expression)
System.Linq.Queryable.Single[TSource](IQueryable1 source, Expression1 predicate)
MusicStore.Models.ShoppingCart.GetTotal() in ShoppingCart.cs
117. var album = _db.Albums.Single(a => a.AlbumId == item.AlbumId);
MusicStore.Controllers.ShoppingCartController.Index() in ShoppingCartController.cs
25. var viewModel = new ShoppingCartViewModel
--- End of stack trace from previous location where exception was thrown ---
Microsoft.AspNet.Mvc.ReflectedActionExecutor.d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Mvc.ReflectedActionExecutor.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Mvc.ReflectedActionInvoker.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Mvc.ReflectedActionInvoker.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
Microsoft.AspNet.Mvc.ReflectedActionInvoker.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
Microsoft.AspNet.Mvc.MvcRouteHandler.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Routing.Template.TemplateRoute.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Routing.RouteCollection.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Builder.RouterMiddleware.d__1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Microsoft.AspNet.Security.Infrastructure.AuthenticationMiddleware1.<Invoke>d__1.MoveNext() --- End of stack trace from previous location where exception was thrown --- System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNet.RequestContainer.ContainerMiddleware.<Invoke>d__1.MoveNext() --- End of stack trace from previous location where exception was thrown --- System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) Microsoft.AspNet.Diagnostics.ErrorPageMiddleware.<Invoke>d__1.MoveNext()

[anpete]

@kirthik, @rowanmiller This is because we don't have the ability to load nav props yet:

// TODO Collapse to a single query once EF supports querying related data
decimal total = 0;
foreach (var item in _db.CartItems.Where(c => c.CartId == ShoppingCartId))
{
    var album = _db.Albums.Single(a => a.AlbumId == item.AlbumId);
    total += item.Count * album.Price;
}

[rubenalves]

Is the ability to load nav props planned for the beta or alphaX ?

[rowanmiller]

It's down as a 'nice to have' for beta - #199.

Any connection strings that we scaffold will have MARS enabled and we can also look at wrapping the exception from SqlClient in another exception that mentions enabling MARS.

[rowanmiller]

I just also wanted to note that the behavior of requiring MARS for this pattern of querying is that same as it has been in past versions of EF too (though the pattern is more common until we support eager loading - a.k.a. Include)

[kirthik]

It might be helpful for users to understand when MARS is required. So adding notes/blog to explain the scenarios that need MARS will be useful I think.

[rowanmiller]

Given the SQL exception isn't very helpful, one thing we could do is wrap the exception with something that explains what MARS is, why it's need it, and how to enable it.

The downside is that this would be messy/intrusive to implement. Within our code, the exception occurs in Microsoft.Data.Entity.Relational.Query.QueryMethodProvider+Enumerable<T>+Enumerator.MoveNext(). We'd probably want to do this at the SQL Server layer (rather than relational) so we'd have to have SQL specific implementations of these components. I haven't dug into it much further, but it seems like it would be kinda messy. @anpete thoughts?

[rowanmiller]

Triage: We should do this on the DatabaseErrorPage

[sayedihashimi]

Hopefully we can come with a design that will not require an explicit MARS attribute in the connection string. By default when publishing to Azure Web Sties we should have a flow that doesn't produce errors. The connection string for SQL Azure is constructed by Azure so we don't have much control over the MARS attribute. We can work with Azure folks to include MARS in connection strings that are generated but that would apply to all new connection strings generated (i.e. even for non vNext sites). I'm afraid that may introduce regressions to non-vNext customers.

[sayedihashimi]

Any thoughts about how we can get this to work for Preview w/o having to change all new Azure connection strings? I'm concerned about impacting asp.net vCurrent scenarios. Is there a template change that we can make here to default MARS or something?

cc @glennc @danroth27 @rustd @mayurid @barrytang

[anpete]

We could turn it on automatically for SQL Server connections that we open. This seems reasonable given we require MARS, and we own opening the connection.

[rowanmiller]

I agree this would be good. Not sure if we can detect if it was explicitly turned off in the connection string and avoid enabling it in those scenarios. Or we could just have UseSqlServer add it to the connection string if it's not already there.

[sayedihashimi]

Thanks @anpete and @rowanmiller. Thanks, can we get something in place for _Preview_?

[rowanmiller]

@divega @ajcvickers - Thoughts on this one? Do you think just adding MARS=true to the connection string in UseSqlServer if it isn't already included would be reasonable? It seems little wrong to mess with the developers connection string, but if they haven't explicitly disabled it then I think the benefit is probably worth the 'wrongness' of it.

[ajcvickers]

I've been thinking about it this morning. It feels wrong to change the connection options that a user has specified and if we can make it easier for them to get it right in the first place, or direct them how to get it right when it fails, then this seems better. Also, as we move forward there is benefit in not requiring MARS both so that this is not hit and so that query is fully functional with providers that don't support MARS. However, query has many competing constraints and this may be an unrealistic goal. On the other hand, if we know (or can be reasonably sure) that the application is going to fail without MARS, then turning it on with a warning may be better than failing. I say, "may", because this is a development/test scenario. If it fails and the user is given good guidance to switch it on, then ultimately this single dev/test failure may be better than perpetually masking the issue. So I think, all in all, it's worth considering this option for the future, but I don't think we should do it now.

[rowanmiller]

The main issue Sayed is referring to is actually a "production" issue as the connection strings we create for local dev includes MARS but when it is overridden to a SQL Azure connection during deployment the new connection string does not have MARS.

@sayedihashimi we already have a bunch of special casing around this connection string... any chance we could just add on MARS as part of that?

[ajcvickers]

So if there is no way of testing in an environment that mirrors production without actually going to production, then that seems like a bigger issue.

[anpete]

@ajcvickers Disagree. Most users won't care about MARS or even know what it is. This is a pit of success thing and being able to explicitly disable MARS doesn't actually seem to achieve anything - Queries that require MARS will fail, and any queries that don't won't use MARS anyway. We can and should log that we have enabled MARS automatically.

[rowanmiller]

I think there is always a danger that your not going to know if the production connection string (or database in general) may hit something different that dev/test until you actually plug them in.

That said, I don't think this right click -> publish... workflow is something customers with a dev/test/prod flow are really going to use. I think it's more for demos and for folks who have a small project and the impact of an outage doesn't outweigh the ease of deploying straight from dev -> prod from VS.

Bigger solutions are going to spend more time setting up a proper test environment that is as close as possible to prod.

[divega]

@sayedihashimi Just to add to what @rowanmiller said, I remember a few years ago when SQL Azure started without support for MARS we made changes to the EF Designer to not enable MARS in new connection strings by default. But that turned out to be a temporary limitation and as soon as MARS became supported we enabled it back. SQL Azure Federations did not support MARS but we are not recommending customers to use SQL Azure Federations anymore. In summary, I am not aware of any strong reason MARS should be disabled with SQL Azure.

Could the deployment tools be updated to generate a different connection string? It is not great that an application that was developed using a connection with MARS enabled (whether it was a user choice or a choice made automatically by tooling) suddenly gets deployed with a connection that isn't MARS enabled. Regardless of whether EF is in the picture things will tend to be broken.

Besides that, I also agree with @ajcvickers that for the long run we should consider not requiring MARS in EF7 by default. But we cannot make this change in the short term.

Fiddling with a connection string makes me nervous, because in the past this has been a mess in several scenarios, e.g. for connections that were previously open and necessary credentials got removed, the impact on connection pooling, etc. If are convinced it is important to do something in EF for beta I vote for us throwing a clear exception when we get passed a SQL Server connection string that doesn't have MARS = true in it.

[rowanmiller]

Sounds like we need to meet to come to a consensus on this one, I'll put something together for the next design meeting.

[sayedihashimi]

I spoke to @kirthik about this about a week ago regarding if the Visual Studio web publish dialog should do something. Below are my comments.

Let’s take a step back for a second, in ASP.NET 5 the publish dialog does not currently accept a connection string. When running a website in Azure if you create a site in the publish dialog with a linked DB there is a connection string added to an env var that makes it available to the app at runtime. The result when creating the site+db in VS is the same as when you do that same thing in the portal.

That said at some point VS will have a connection string update feature. The design is TBD but we are currently thinking to write a config.override.json file that is imported via runtime. When running in Azure websites if we write the file (which we may not for azure websites, tbd) that value would be ignored since there is an env var with the exact same name for the linked db. Even if we added MARS=true in config.override.json it wouldn’t matter because the Azure env var will win.

To summarize; the bug is not currently a bug since we don’t update connection strings and when we do handle connection strings there is nothing we can do for azure websites since the env var for the linked DB will trump anything that VS can do.

If you don’t think it makes sense to enable MARS=true for all of Azure then I would recommend you explore other options. One thing that I can think of is having an appSetting for MARS and having that appSetting to default to true in VS templates.

[anpete]

Closing as we no longer require MARS.