U4-2172 - Simple fix for random PetaPoco related errors in a multithreaded Umbraco Instance

Created by Ron Brouwer 01 May 2013, 14:34:27 Updated by Sebastiaan Janssen 30 May 2013, 07:24:54

Is duplicated by: U4-2293

Hi,

Every now and then we encounter random PetaPoco related errors like: System.InvalidOperationException: There is already an open DataReader associated with this Command which must be closed first. System.InvalidOperationException: ExecuteReader requires an open and available Connection. The connection's current state is connecting. Because of these errors we frequently had missing DOCTYPE elements in the umbraco.config resulting in null values returning from Content.GetElementById.

After a long search I've found the problem. The errors may occour when multiple threads are using the database without a HttpContext. (in our case it was the refreshcontentfromdatabaseasync and some custom async process)

The documentation of PetaPoco states that you should use a database instance per request or per thread, however in Umbraco a global instance is used when there is no HttpContext. Causing one thread to potentially close a dbconnection used by some other thread.

Note: for transactions to work, all operations need to use the same instance of the PetaPoco database object. So you'll probably want to use a per-http request, or per-thread IOC container to serve up a shared instance of this object.

'''You can fix this very simple by using one instance per thread in Umbraco.Core.Persistence.DefaultDatabaseFactory using the ThreadStatic attribute:'''

    [ThreadStatic]
    private static UmbracoDatabase _threadInstance = null;
public UmbracoDatabase CreateDatabase()
{
        if (_threadInstance == null)
        {
            _threadInstance = string.IsNullOrEmpty(_providerName) == false && string.IsNullOrEmpty(_providerName) == false
                                  ? new UmbracoDatabase(_connectionString, _providerName)
                                  : new UmbracoDatabase(_connectionStringName);
        }
        return _threadInstance;
}

'''and remove the original code:'''

private static volatile UmbracoDatabase _globalInstance = null;
public UmbracoDatabase CreateDatabase()
{
	//no http context, create the singleton global object
	if (HttpContext.Current == null)
	{
		if (_globalInstance == null)
		{
			lock (Locker)
			{
				//double check
				if (_globalInstance == null)
				{
				    _globalInstance = string.IsNullOrEmpty(_providerName) == false && string.IsNullOrEmpty(_providerName) == false
				                          ? new UmbracoDatabase(_connectionString, _providerName)
				                          : new UmbracoDatabase(_connectionStringName);
				}
			}
		}
		return _globalInstance;
	}
	//we have an http context, so only create one per request
	if (!HttpContext.Current.Items.Contains(typeof(DefaultDatabaseFactory)))
	{
		HttpContext.Current.Items.Add(typeof (DefaultDatabaseFactory),
                                         string.IsNullOrEmpty(_providerName) == false && string.IsNullOrEmpty(_providerName) == false
			                              ? new UmbracoDatabase(_connectionString, _providerName)
			                              : new UmbracoDatabase(_connectionStringName));
	}
	return (UmbracoDatabase)HttpContext.Current.Items[typeof(DefaultDatabaseFactory)];
}

Hope this helps. Thanks!

Ron Brouwer

Comments

Shannon Deminick 02 May 2013, 12:52:38

That's brilliant... TBH I had no idea that this was possible with just that attribute! I've been looking for a way to "piggy-back" a static object on a thread... and there it is. Nice :) I feel dumb :P


Shannon Deminick 02 May 2013, 13:41:36

... PS though the problem really is from the method refreshcontentfromdatabaseasync which will eventually be refactored.


Shannon Deminick 02 May 2013, 13:44:04

Also, I'm going to go with a combined approach based on this thread: http://www.hanselman.com/blog/ATaleOfTwoTechniquesTheThreadStaticAttributeAndSystemWebHttpContextCurrentItems.aspx

So for non-http instances, we'll use ThreadStatic, for Http instances, we'll use the items collection


Shannon Deminick 02 May 2013, 13:48:25

Fixed in ffc9e91cea67


Ron Brouwer 06 May 2013, 09:21:50

Thanks for fixing! I'm not sure it's needed to use the Http storage, I think it's fine to reuse the same db instance for multiple requests as long as only one thread is using it at a time. But it'll work anyway so thanks!


Priority: Critical

Type: Exception

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Inline code

Affected versions: 6.0.0, 6.0.1, 6.0.3, 6.0.4, 6.1.1, 6.0.6

Due in version: 6.1.0, 6.0.6, 4.11.9

Sprint:

Story Points:

Cycle: