Soap WebService with Database access

Question

So basically I have a Soap webservice that does inserts and retrieves some data from a SqlServer database.

The webservice uses a Singleton that is responsible for the DB stuff.

public class Service : System.Web.Services.WebService
{
    private DBAccess dbaccess;
    public Service()
    {
        dbaccess = DBAccessLocalhost.GetInstance();
    }

    [WebMethod]
    public List<Profile> XXX(Guid a, uint b, DateTime c)
    {
        return dbaccess.XXX(a, b, c);
    }
    ...
}

The singleton that access the database. It has allot of methods that basically do this.

public class DBAccessLocalhost : DBAccess
{
    private static DBAccess instance = null;
    private string connectionString;

    public static DBAccess GetInstance()
    {
        if (instance == null)
            instance = new DBAccessLocalhost();

        return instance;
    }

    private DBAccessLocalhost()
    {
        connectionString = "Data Source=localhost;Initial Catalog=DBName;Integrated Security=True;Max Pool Size=2000;Pooling=false";
    }

    public override void XXX(Guid a, uint b, DateTime c)
    {
        SqlCommand cmd;
        SqlDataReader dr;
        string strSql = "SP_Name";
        SqlConnection conn;
        conn = new SqlConnection(connectionString);

        try
        {
            conn.Open();

            cmd = new SqlCommand(strSql, conn);
            cmd.CommandType = System.Data.CommandType.StoredProcedure;

            cmd.Parameters.AddWithValue("@a", a.ToString());
            cmd.Parameters.AddWithValue("@b", (int)b);
            cmd.Parameters.AddWithValue("@c", c);
            dr = cmd.ExecuteReader();

            while (dr.Read() && dr.HasRows)
            {
                //Do stuff...
            }

            dr.Close();

        }catch (Exception ex)
        {
            throw new DBError("Errors: " + ex.Message);
        }
        finally
        {
            conn.Close();
        }
     }

Second version:

public class DBAccessLocalhost : DBAccess
{
    private static DBAccess instance = null;
    private string connectionString;

    public static DBAccess GetInstance()
    {
        if (instance == null)
            instance = new DBAccessLocalhost();

        return instance;
    }

    private DBAccessLocalhost()
    {
        connectionString = "Data Source=localhost;Initial Catalog=DBName;Integrated Security=True;Max Pool Size=2000;Pooling=true";
    }

    public override void XXX(Guid a, uint b, DateTime c)
    {
        string strSql = "SP_Name";

        using (SqlConnection conn = new SqlConnection(connectionString))
        {
            using (SqlCommand cmd = new SqlCommand(strSql, conn))
            {
                cmd.CommandType = System.Data.CommandType.StoredProcedure;

                cmd.Parameters.AddWithValue("@a", a.ToString());
                cmd.Parameters.AddWithValue("@b", (int)b);
                cmd.Parameters.AddWithValue("@c", c);

                try
                {
                    conn.Open();

                    using (SqlDataReader dr = cmd.ExecuteReader())
                    {
                        while (dr.Read())
                        {
                            //....
                        }
                    }
                }
                catch (Exception ex)
                {
                    throw new DBError("Error: " + ex.Message);
                }
                finally
                {
                    conn.Close();
                }
            }
        }

The problem is that sometimes I get this exception:

 DBAccessWebSerice,System.ServiceModel.FaultException: Server was unable to process 
 request. ---> Errors: ExecuteNonQuery requires an open and available    
 Connection. The connection's current state is closed (Sometimes says connecting).

 Server stack trace: 
 at System.ServiceModel.Channels.ServiceChannel.HandleReply(ProxyOperationRuntime 
 operation, ProxyRpc& rpc)
 at System.ServiceModel.Channels.ServiceChannel.Call(String action, Boolean oneway, 
 ProxyOperationRuntime operation, Object[] ins, Object[] outs, TimeSpan timeout)
 at System.ServiceModel.Channels.ServiceChannelProxy.InvokeService(IMethodCallMessage  
 methodCall, ProxyOperationRuntime operation)
 at System.ServiceModel.Channels.ServiceChannelProxy.Invoke(IMessage message)

The problem may be because there are to many connections at the same time to the database?

Probably this isn't the best way do to this. But if you have a better way of doing this or have a solution for this problem, please say.

Answer 1

Singleton is not a good pattern for Db Access. Use static methods for retrieving data. Most likely, your problem is that 2 threads accessed your instance and you have no lock to prevent it. Other points:

1. don't name your stored procedures with "SP_" prefix
2. use using while dealing with disposable objects such as datareader
3. Use connection pooling

I see potential for all sorts of issues in the code you presented here. For example, if reading datareader errors out, you will not clean resources.

This is one way:

public override void XXX(Guid a, uint b, DateTime c)
{
    string strSql = "SP_Name";
    lock (lockObject) {  // lock the code for the duration of execution so it is thread safe now (because you in singleton) 
// But this is bad for DB access !!!! What if SP executes long - other threads have to wait
// Use simple static methods for data access and no `lock`
        using (SqlConnection conn = new SqlConnection(connectionString))
        {
            conn.Open();
            using (SqlCommand cmd = new SqlCommand(strSql, conn))
            {
                cmd.CommandType = System.Data.CommandType.StoredProcedure;

                cmd.Parameters.AddWithValue("@a", a.ToString());
                cmd.Parameters.AddWithValue("@b", (int)b);
                cmd.Parameters.AddWithValue("@c", c);
                // no need for any `try-catch` here because `using` is that and `finally`
                using (SqlDataReader dr = cmd.ExecuteReader())
                {
                    while (dr.Read())
                    {
                        //....
                    }
                }

            }
            conn.Close();
        }
    }
}

Other option, if you want to have better control of exception handling is to use try-catch-finally instead of using. Then, in finally, you will do another try-catch to clean-up down your resources. //Pseudocode

try
       connection
       command
       datareader
   catch (Exception ex)
       prepare user message
       log or
       throw/throw new, etc // depends. This is Webservice, may be log and response with error message
   finally
       try
          if datareader alive
             close datareader 
             dispose datareader 
       catch // do nothing here
       try
          if command alive
             dispose command
       catch // do nothing here
       try
          if connection alive
             close connection 
             dispose connection 
       catch // do nothing here

Answer 2

Here's an approach, abandoning the singleton idea as the other answer suggests:

• Pass a Func into your service which is the factory for data access object

• Have DBAccess implement IDisposable which and use a using block

  public class Service : System.Web.Services.WebService
  {
     private Func<DBAccess> getDBAccess;
  
     public Service(Func<DBAccess> getDBAccess)
     {
        this.getDBAccess = getDBAccess;
     }
  
     [WebMethod]
     public List<Profile> XXX(Guid a, uint b, DateTime c)
     {
          using(var dbaccess = this.getDBAccess())
          {
              return dbaccess.XXX(a, b, c);
          }  
     }
     ...
   }