Sign in to follow this  
GuyWithBeard

Is this singleton thread safe?

Recommended Posts

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!

Share this post


Link to post
Share on other sites
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 ;)

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this