Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document connection obtained from DatabaseFacade.GetDbConnection() should normally not be disposed #11415

Closed
ststeiger opened this issue Mar 24, 2018 · 12 comments · Fixed by #22626
Labels
area-docs area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@ststeiger
Copy link

ststeiger commented Mar 24, 2018

I have a question:
I wrote an extension method to execute a stored procedure.

Now, I use DatabaseFacade.GetDbConnection() to get a db connection.
So far so good, everything is working.
Had a little trouble with getting the factory from the connection, but solved that.

When testing, the procedure ran fine, but I noticed I get a NULL-reference exception on queries afterwards.

I first thought it was due to my provider extensions, but then I noticed, as soon as you do

using(var foo = this.m_ctx.Database.GetDbConnection())
{
   // Don't even have to do something with it
    System.Console.WriteLine(fo9);
}

this happend.

Is that a bug in EF, or am I, for some odd reason, not supposed to dispose the connection ?

private System.Collections.Generic.List<T_custom_values>
FetchCustomFieldsValues(string customized_type, int id)
{
    // If I take using here, I get NULL-Reference on custom_fields.Max
    //using (
    var fo = this.m_ctx.Database.GetDbConnection();
    {
        System.Console.WriteLine(fo);
    }


    int max = this.m_ctx.custom_fields.Max(x => x.id);

    System.Console.WriteLine(this.m_ctx.custom_fields);
    // [...] etc. 
    //return something;
}

@ajcvickers
Copy link
Member

@ststeiger GetDbConnection provides access to the DbConnection that EF is using internally. Application code should not dispose it.

@ErikEJ
Copy link
Contributor

ErikEJ commented Mar 26, 2018

@ajcvickers Improve docs?

@ajcvickers
Copy link
Member

@divega?

@divega
Copy link
Contributor

divega commented Mar 26, 2018

@ajcvickers I created dotnet/EntityFramework.Docs#665, however it seems to me that the most direct way to improve the documentation of this API would be to add information to the API's XML docs. Thoughts?

@ststeiger
Copy link
Author

Why doesn't it just create a new connection ?
And if it is used internally, and may not be disposed externally, why do you expose it externally ?

Or maybe more to the point:
Why is there no public method which allows me to get a (new) raw connection (with connection string already initialized) and the System.Data.Common.DbProviderFactory ? Or am I just not seeing it ?

While it's nice to have compile-time checking on SQL-field presence, I think there are a lot of cases where EF just doesn't quite cut it. There, I'd like to make due with Dapper, but I don't want to manage the abstraction/DbFactory and the connection string more than once, especially if EF already does it for me.

@ajcvickers
Copy link
Member

@divega Agreed--suggest we move it to the backlog for post 2.1.

@ajcvickers ajcvickers reopened this Mar 28, 2018
@ajcvickers
Copy link
Member

@ststeiger The intention is that the EF context acts as essentially your unit-of-work. The connection is exposed so that you can integrate both EF calls and lower-level ADO.NET calls into that unit of work using the same connection.

Using EF as a factory for connections outside of this is not something we have heard people asking for in the past. It's probably something that EF could do, but it's not clear to me that a) it adds significant value and b) that it is an appropriate responsibility for EF to have. However, we will discuss it in triage and decide appropriately.

@ajcvickers
Copy link
Member

Triage: moving to the backlog to update API docs to indicate that the connection obtained in this way should not be disposed.

With regard to making EF a factory for connections, we decided not to do this because:

  • It will only work if the context is configured with a connection string, as opposed to an existing connection. (If all EF has is a connection, then this becomes the connection clone problem, which is definitely something EF should not get back into doing.)
  • Given that this then becomes purely about creating a DbConnection given a connection string having EF do it adds very little value.
  • A simple extension method could be written by the application to do this. It would need to create a context, then get the connection and return it without disposing the context. It is likely possible to do it more cleanly, if desired, using EF's internal services.

@John0King
Copy link

@ajcvickers where's the doc and what's the difference when work with external connection ?

@ajcvickers
Copy link
Member

@John0King Update the API docs means update the API documentation in the source code, which hasn't been done yet, hence this issue is still open. With regard to external connections, if you create the DbConnection and pass it to EF, then you must dispose of it after the context has been disposed. If EF creates the DbConnection object, then you must not dispose of it.

@John0King
Copy link

An existing <see cref="T:System.Data.Common.DbConnection" /> to be used to connect to the database. If the connection is in the open state then EF will not open or close the connection. If the connection is in the closed state then EF will open and close the connection as needed.

@ajcvickers Does this mean it will be the same as we pass a connection string to the .UseSqlServer() method if we pass a DbConnection in close state ?

@ajcvickers
Copy link
Member

@John0King Yes.

@ajcvickers ajcvickers modified the milestones: Backlog, MQ Sep 11, 2020
@ajcvickers ajcvickers self-assigned this Sep 16, 2020
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Sep 20, 2020
@ajcvickers ajcvickers changed the title Disposing DatabaseFacade.GetDbConnection() yields NULL-reference in future queries Document connection obtained from DatabaseFacade.GetDbConnection() should normally not be disposed Sep 20, 2020
ajcvickers added a commit that referenced this issue Sep 20, 2020
* Document connection obtained from DatabaseFacade.GetDbConnection() should normally not be disposed Fixes #11415
* Add XML docs referencing how to determine the default CommandTimeout Fixes #17503
* Clarify behavior for EnsureExists with an empty database Fixes #17563
* A hyperlink to the DbContext.Database.Migrate() method would be useful Fixes #17571
* Default values for maxRetryCount, maxRetryDelay, and errorNumbersToAdd Fixes #17574
* Document that modifying entity states while iterating over entries can result in "Collection was modified" exception Fixes #18389
* Update API doc links to correctly reference external dependencies Fixes #18580
* Make it clearer how to access EF.Functions Fixes #21424
* Improve API docs for TrackGraph Fixes #22529
ajcvickers added a commit that referenced this issue Sep 21, 2020
* Document connection obtained from DatabaseFacade.GetDbConnection() should normally not be disposed Fixes #11415
* Add XML docs referencing how to determine the default CommandTimeout Fixes #17503
* Clarify behavior for EnsureExists with an empty database Fixes #17563
* A hyperlink to the DbContext.Database.Migrate() method would be useful Fixes #17571
* Default values for maxRetryCount, maxRetryDelay, and errorNumbersToAdd Fixes #17574
* Document that modifying entity states while iterating over entries can result in "Collection was modified" exception Fixes #18389
* Update API doc links to correctly reference external dependencies Fixes #18580
* Make it clearer how to access EF.Functions Fixes #21424
* Improve API docs for TrackGraph Fixes #22529
ajcvickers added a commit that referenced this issue Sep 24, 2020
* Update API docs

* Document connection obtained from DatabaseFacade.GetDbConnection() should normally not be disposed Fixes #11415
* Add XML docs referencing how to determine the default CommandTimeout Fixes #17503
* Clarify behavior for EnsureExists with an empty database Fixes #17563
* A hyperlink to the DbContext.Database.Migrate() method would be useful Fixes #17571
* Default values for maxRetryCount, maxRetryDelay, and errorNumbersToAdd Fixes #17574
* Document that modifying entity states while iterating over entries can result in "Collection was modified" exception Fixes #18389
* Update API doc links to correctly reference external dependencies Fixes #18580
* Make it clearer how to access EF.Functions Fixes #21424
* Improve API docs for TrackGraph Fixes #22529

* Review feedback

* Update src/EFCore/EF.cs

Co-authored-by: Shay Rojansky <roji@roji.org>

Co-authored-by: Shay Rojansky <roji@roji.org>
@ajcvickers ajcvickers modified the milestones: MQ, 6.0.0 Nov 25, 2020
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview1 Jan 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-docs area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants