Back to General and Gameplay Programming

Is this singleton thread safe?

General and Gameplay Programming Programming

Started by GuyWithBeard March 22, 2010 01:23 AM

6 comments, last by LorenzoGatti 14 years, 1 month ago

GuyWithBeard

1,932

Author

March 22, 2010 01:23 AM

I have searched the internet for some C++ singleton patterns, and have come across some that are mentioned as thread safe and some non thread safe, but I have not really understood what it is that makes a certain pattern thread safe. I have, in my program, a Database class that looks like this; Header:

class Database
{
private:
	sql::Driver* mDriver;
	sql::Connection* mConnection;
	
	static Database* smInstance;
	
	Database();
		
public:
	static Database* getInstance();
	~Database();
	
	bool connect(NSString* host, NSString* username, NSString* password, NSString* database);
};

Source:

Database* Database::smInstance = 0;

Database* Database::getInstance()
{
	if(!smInstance)
	{
		smInstance = new Database();
	}
	
	return smInstance;
}

Database::Database()
{
	mDriver = get_driver_instance();
	
	mConnection = 0;
}

bool Database::connect(NSString* host, NSString* username, NSString* password, NSString* database)
{
	try
	{
		mConnection = mDriver->connect([host UTF8String], [username UTF8String], [password UTF8String]);
	
		sql::Statement* stmt = mConnection->createStatement();
	
		stmt->execute(std::string("USE ") + std::string([database UTF8String]));
		
		delete stmt;
	}
	catch (...)
	{
		return false;
	}
	
	return true;
}

Database::~Database()
{
	if(mConnection) delete mConnection;
}

Do you see any problems with this singleton pattern? Can this be considered thread safe? Thanks!

Hodgman

52,717

March 22, 2010 01:53 AM

No it's not thread safe. For example, if two threads call getInstance at once, it's possible for two 'Database' objects to be constructed, one of which will be leaked.

	Lock a Mutex/Critical-section Here	if(!smInstance)	{		smInstance = new Database();	}	Unlock the Mutex/Critical-section Here	return smInstance;

There is a "pattern" called double-checked locking which is supposed to make your getInstance function thread-safe, however, without intimate knowledge of memory fences and atomic operations most implementations of double-checked locking are still actually NOT thread safe. For this reason it's become known as an "anti-pattern". Better to just surround the getInstance function with a lock of some kind instead of attempting double-checked locking.

Apart from worrying if the singleton itself is thread-safe, is your Database object thread-safe? Because if it's not, then it doesn't matter if the singleton is ;)

. 22 Racing Series .

Ravyne

14,306

March 22, 2010 01:57 AM

This implementation doesn't seem to be any different than the typical, bog-standard singleton implementation that is not thread safe.

More to the crux of the matter, why do you believe that a singleton has any value here? What are you attempting to gain?

I can imagine that software might like to have multiple database connections, even to the same database, for different purposes, perhaps with different security and permissions on each connection.

Even if you believe your application to only "need" one connection (and setting aside the typical singleton argument of "need - read 'want'" and "need - read 'otherwise impossible'") you still can't guarantee that its state is fully under the control of your program -- it is a database, after-all, and some other connection from some other program, or some user who's logged on and editing tables, can still change the database outside the control of your singleton. You can probably limit this through your database configuration, but then the singleton in your code serves no purpose at all, because the server itself will only allow one connection.

I'd recommend you to re-evaluate your 'need' for the singleton before worrying about whether its implementation is correct.

throw table_exception("(? ???)? ? ???");

GuyWithBeard

1,932

Author

March 22, 2010 02:56 AM

Nice comments from both of you, thanks! I had actually not though about the thread safety of the actual class. I would probably need some kind of locking mechanism to allow only one thread of my application to access it at a certain time.

I originally made the class a singleton because, as you suggested, I think my app only "needs one connection" to the DB. I am not going to write any explicitly multi-threaded code myself, but I am working in Cocoa, and I am not exactly sure if Cocoa creates multiple threads or not, so I though I would try to make this thread safe. Having the class as a singleton is handy since you dont need to carry around a pointer to the database, instead you can access it directly through the getInstance mehtod whenever you want.

I also know that I cannot be sure of the database state, but that is not my intention. MySQL should be responsible for handling multiple concurrent requests/updates, right?

Maybe I should create connections to the database "on demand", i.e. when needed in the program, and then close those connections when they have done their thing, e.g. performed a request/update. Any thoughts on this approach?

taz0010

277

March 22, 2010 03:36 AM

I'd maintain a single database connection throughout the lifetime of the program, and I wouldn't use singletons. I choose to pass pointers around instead, and I feel that doing so has encouraged much better design than a system with global state where you can access anything from anywhere. And your threading problems will disappear if you aren't using "construct on demand", so you should be better off without singletons in this case.

samoth

9,833

March 22, 2010 04:23 AM

One could say that the question "is this singleton threadsafe" or the desire to write a threadsafe singleton is ill-formed.

A singleton usually does not really need to be thread-safe, and should not, as it is really hard to get a good, efficient, and reliable implementation. On the other hand, it is usually perfectly acceptable to simply instantiate (and, if necessary de-instantiate) the singleton inside main(), before the application calls a function that spawns any threads.
Having said that, it is often not necessary at all to have a singleton in the first place, but instantiating a single instance in a defined manner is good enough in most cases. Few things really have to be singletons.

the_edd

2,109

March 22, 2010 07:56 AM

Quote:Original post by GuyWithBeard
I originally made the class a singleton because, as you suggested, I think my app only "needs one connection" to the DB.

This should work for most databases: give each thread its own connection. Do check, but even sqlite supports this kind of usage.

Alternatively, wrap a single connection in a multi-producer-single-consumer message queue and interact with the database through that queue.

http://www.mr-edd.co.uk
http://bitbucket.org/edd

Antheus

2,410

March 22, 2010 08:12 AM

Singleton can be made thread-safe, but not for much benefit. Its members, by definition, cannot be made reentrant.

Double-checked locking is a pattern that relies on concepts which are quite difficult to achieve in C++ due to most of it relying on hardware details. It was even broken in Java for a decade.

Finally, there is a linking problem, which may result in race condition even without presence of threads. From design perspective, database singleton is inherently stateless. Data is stored elsewhere, connections, due to managed nature, are transient. This implies there is nothing that warrants "just existing", or at very least is a promise that simply cannot be kept.

When talking about concurrency - 12 cores are entering the market, some machines run 4096 cores today in single address space. "Works for me on dual core" is no longer a valid justification.

As far as having one database only - this is the very first fallacy anyone encounters. IT world learned as far back as 2000 why this is a disaster and dropped singletons like old news for this purpose.

There are two design and management problems:
- Swapping test for production database - if everything is hard-coded to one-and-only this becomes impossible
- Configuration and other initialization issues. Things like passing in authentication token or dynamically configuring database location cause no end to complications.

And as a hard rule - unless it is written, in blood, with contract, in detail - it is not thread-safe nor reentrant. Proving these two is very trivial, without even touching on ACID guarantees.

LorenzoGatti

4,648

March 22, 2010 12:06 PM

Quote:Original post by GuyWithBeard
Maybe I should create connections to the database "on demand", i.e. when needed in the program, and then close those connections when they have done their thing, e.g. performed a request/update. Any thoughts on this approach?

Yes, that's the right way, for reasons that have already been mentioned (separation between threads, simpler design, leaving concurrency and locking policies to the DBMS where they belong, the possible need for different connection types).

If you find a good one for the DBMS and other libraries you are using, you might want to adopt a connection pooling library that quickly recycles old connections (in a thread-safe manner, of course) instead of creating completely new ones for each request and tearing them down; it is a rather unnecessary optimization.

Omae Wa Mou Shindeiru

Is this singleton thread safe?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Is this singleton thread safe?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines