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 correct usage of DatabaseContext #745

Closed
KevinJump opened this issue Apr 17, 2018 · 7 comments
Closed

Document correct usage of DatabaseContext #745

KevinJump opened this issue Apr 17, 2018 · 7 comments

Comments

@KevinJump
Copy link
Contributor

@KevinJump KevinJump commented Apr 17, 2018

From the latest release to Umbraco 7.8/7.9/7.10 to address a memory leak
http://issues.umbraco.org/issue/U4-11207

in the issue and comments - @zpqrtbnk states:

Directly using DatabaseContext.Database is strongly discouraged, and will in fact be impossible in v8.

and in comment :

Duh, need to update the docs then. Best practice:

using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope())
{
  var database = scope.Database;  
  // use database  
  scope.Complete();
}

The Common Pitfalls page actually shows an good practice code snippet using DatabaseContext.Database - it would be good if we could have definitiave sample of what is good practice here ? (see https://github.com/umbraco/UmbracoDocs/blob/master/Reference/Common-Pitfalls/index.md#usage-of-singletons-and-statics)

@dampee

This comment has been minimized.

Copy link
Contributor

@dampee dampee commented Apr 17, 2018

Thanks @KevinJump
Definitely very valid feedback. Don't hesitate to do any pull request.

@mvanhelmont

This comment has been minimized.

Copy link

@mvanhelmont mvanhelmont commented Apr 17, 2018

@KevinJump I am also interested, I see ScopeProvider as internal in the ApplicationContext. So maybe core can show the correct way?

@Shazwazza

This comment has been minimized.

Copy link
Member

@Shazwazza Shazwazza commented Apr 18, 2018

Some clarification is needed and we don't want to confuse everyone ... Using the DatabaseContext.Database is fine in almost all scenarios except for when running code on background threads. Anywhere that we expose the DatabaseContext on base classes, it's totally fine to use it there. I don't think there's a need to update this particular doc because it's usage is fine.

We should update the docs with regards to running code on background threads and in those cases some care will need to be taken. It is possible to be consistent however and access the database using the ScopeProvider in all scenarios, so if you want to be consistent than you can be. In which case we should also expose the ScopeProvider on base classes directly where appropriate.

I also wish to point out that using singletons like ApplicationContext.Current should be avoided where possible .... just like the doc you've referenced mentions :)

I just don't want everyone to suddenly think that everything you've been doing is wrong. @zpqrtbnk let's try not to confuse everyone :) Yes in v8 things will be different but we aren't using v8 right now.

@mvanhelmont

This comment has been minimized.

Copy link

@mvanhelmont mvanhelmont commented Apr 18, 2018

That’s make things clear. Currently i have issues with NoScope on a Transaction and when using scopeprovider it works.

http://issues.umbraco.org/issue/U4-10290?preventRedirect=true

This is inside of a apicontroller so no background tread. Is this a valid scenario? Or is using Transaction oké but it’s realy a bug at the moment!

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Apr 18, 2018

Comments:

  • re. ApplicationContext.Current: yes, this is bad practice, one should not use singletons, so make it applicationContext and assume it has been passed by whatever is calling the code
  • re. DatabaseContext.Database: let's say I got confused by the current v8 works (and overlooked the fact that the scope provider is still internal in v7); current documentation is fine, but might need to be complemented with the following details:

DatabaseContext.Database gets its value from an underlying "scope" which controls a global database transaction. Umbraco maintains such a scope on each request, so DatabaseContext.Database is totally fine in the context of a request: during front-end requests, in controllers, etc.

OTOH, outside of a request, e.g. in a background thread, there is no current scope and DatabaseContext.Database ends up relying on a NoScope instance. These instances are hard to manage properly, and could leak (as seen in issue U4-11207). The fix for the issue (1) deals with a problem where some of our code was leaking instances and (2) improves how we detect and collect leaking instances.

So, so far, and considering that the scope provider is internal: using DatabaseContext.Database in background threads should be OK. But it's not absolutely totally safe. Short term, I'd update the documentation to mention that, should weird thing happen in a background thread, well... an issue should be filed.

@zpqrtbnk

This comment has been minimized.

Copy link
Contributor

@zpqrtbnk zpqrtbnk commented Apr 18, 2018

As for U4-10290: now reading, will comment on the tracker.

@sofietoft

This comment has been minimized.

Copy link
Contributor

@sofietoft sofietoft commented Sep 15, 2019

Hi!
As we haven't seen any new activity on this issue in while, I'll close it for now.

If you feel it's still relevant, you're welcome to reopen it! 😁
Thanks.

/Sofie

@sofietoft sofietoft closed this Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.