Singleton implementation of a database connection

Question

I have implemented as follows, a class applying singleton pattern to get a global single access to database. I intend to provide a thread-safe implementation.

using System.Data.SqlClient;
public sealed class Database
 {
 private static volatile SqlConnection instance;
 private static object syncRoot = new object();
 private const string connectionString = "Data Source=ServerName;" +
 "Initial Catalog=DataBaseName;" +
 "User id=UserName;" +
 "Password=Secret;";
 private Database() { }
 public static SqlConnection Instance
 {
 get
 {
 if (instance == null)
 {
 lock (syncRoot)
 {
 if (instance == null)
 instance = new SqlConnection(connectionString);
 }
 }
 return instance;
 }
 }
 }

• πάντα ῥεῖ: Did you see this post: stackoverflow.com/questions/3136008/…
• Mathieu Guindon: csharpindepth.com/articles/general/singleton.aspx

Answer 1

As per the docs

https://msdn.microsoft.com/en-us/library/system.data.sqlclient.sqlconnection(v=vs.90).aspx

Any public static (Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe.

This means that while the Singleton instance can be technically correct, it is not correct in this specific context of the SqlConnection class if you plan to access instance members from different threads (and you possibly do).

Answer 2

A connection string is configuration data, it has nothing to do in code, even less so in a const. Pull it out of there, and into a proper configuration settings file. Also consider using Windows Authentication so that you don't need to have passwords stored in plain text anywhere.

What you've made thread-safe is the acquisition of the SqlConnection instance.

About the volatile usage:

Making the instance variable volatile can make it work, as would explicit memory barrier calls, although in the latter case even experts can't agree exactly which barriers are required. I tend to try to avoid situations where experts don't agree what's right and what's wrong!

_{http://csharpindepth.com/articles/general/singleton.aspx}

A better implementation would use a Lazy<T> instead of explicit locks and obscure keywords.

Now, a Singleton normally instantiates itself - so your field and Instance would be a Database object, not a SqlConnection. Database is a very unfortunate name for this concept BTW - your class has nothing to do with a database; if anything it's a SqlConnectionProvider or whatever - but not a Database.

The connection itself can be acquired by any number of threads. What's not clear is who is responsible for correctly disposing that connection.

An SqlConnection implements IDisposable, and should be as short-lived as possible. Seems your intent is to reuse the same connection instance throughout the lifetime of your application, and have some cleanup/exit code deal with proper disposal (if at all?).

The whole idea doesn't feel right at all. The connection overhead is already taken care of with connection pooling, so there's no need to keep a single connection alive forever, even less so to use it across multiple threads.