ADODB Concurrence issue using task c#

Question

I have a for loop and inside the for loop I'm calling a DB method that brings data from SQL server. It is a lot and it takes like 15 sec. I was thinking that maybe I can use task to optimize it.

The problem is when I use the task, and I wait for all task. It's giving me problems related with concurrency, some times that the reader is closed, other issues with the connection.

I have never had the need to think on this scenario, would you mind checking my ADODB code to see if there is a way that I can use the task without having issues of concurrency?

    public  IBudget getBudget(int company, string tablename, int year, string account)
    {
        SqlCommand cmd1;
        int x = 1;
        if (tablename == "Actual")
            x = -1;
        Budget item;
        item = new Budget();
        try
        {
            cmd1 = new SqlCommand("sp_Dashboard_GetBudget", cnxCRM);
            cmd1.CommandType = CommandType.StoredProcedure;
            cmd1.Parameters.AddWithValue("@coNum", company);
            cmd1.Parameters.AddWithValue("@tablename", tablename);
            cmd1.Parameters.AddWithValue("@year", year);
            cmd1.Parameters.AddWithValue("@account", account);
            if( cnxCRM.State == ConnectionState.Closed)
            cnxCRM.Open();
            SqlDataReader sqlDataReader = cmd1.ExecuteReader();                
            if (sqlDataReader.HasRows)
            {
                while (sqlDataReader.Read())
                {

                    item.GLAccount = sqlDataReader["GLAccountNo"].ToString();
                    item.Month1 = float.Parse(sqlDataReader["Month1"].ToString()) * x;
                    item.Month2 = float.Parse(sqlDataReader["Month2"].ToString())* x;
                    item.Month3 = float.Parse(sqlDataReader["Month3"].ToString())* x;
                    item.Month4 = float.Parse(sqlDataReader["Month4"].ToString())* x;
                    item.Month5 = float.Parse(sqlDataReader["Month5"].ToString())* x;
                    item.Month6 = float.Parse(sqlDataReader["Month6"].ToString())* x;
                    item.Month7 = float.Parse(sqlDataReader["Month7"].ToString())* x;
                    item.Month8 = float.Parse(sqlDataReader["Month8"].ToString())* x;
                    item.Month9 = float.Parse(sqlDataReader["Month9"].ToString()) * x;
                    item.Month10 = float.Parse(sqlDataReader["Month10"].ToString())* x;
                    item.Month11 = float.Parse(sqlDataReader["Month11"].ToString())* x;
                    item.Month12 = float.Parse(sqlDataReader["Month12"].ToString()) * x;

                }
            }
        }
        catch (Exception ex)
        {
            if (cnxCRM.State == ConnectionState.Open)
            {
                cnxCRM.Close();
                throw ex;
            }
        }
        finally
        {
            //this.cnxCRM.Close();
        }
        return item;
    }

This is the code where I create the task and wait for all of them. The function data.getBudget is the one that is giving me issues:

   var getBudgetTask = new List<Task>();

        foreach (Checkbook c in list)
        {
            var getAcc = Task.Run(() =>
            {

                List<IAccount> accLst = new List<IAccount>();
                accLst = data.getAccounts(c.CheckbookID).ToList();
                ///for each account get the budget
                foreach (IAccount acc in accLst)
                {
                    IBudget actual = data.getBudget(c.CompanyNumber, "Actual", DateTime.Now.Year, acc.GLAccount);
                    IBudget budget = data.getBudget(c.CompanyNumber, "Budget", DateTime.Now.Year, acc.GLAccount);
                    for (int i = 1; i <= 12; i++)
                    {
                        string name = "Month" + i;
                        final[i - 1].Actual += decimal.Parse(budgetvariable.GetProperty(name).GetValue(actual).ToString());
                        final[i - 1].Budget += decimal.Parse(budgetvariable.GetProperty(name).GetValue(budget).ToString());
                    }
                }
            });

            getBudgetTask.Add(getAcc);
        }

        await Task.WhenAll(getBudgetTask);

Thank you and I will really appreciatte the help.

Side note: Replace `throw ex;` with `throw;`, this way you preserve the stack trace of the original exception. Also you only throw the exception if the connection is open, seems like a logic bug as you should wrap your connectinos in `using` blocks OR close them in the finaly block. — Igor, Sep 20 '17 at 20:04
Create new connection instead of `cnxCRM` inside `getBudget`. Instead of `try-catch` use `using` — T.S., Sep 20 '17 at 20:05
A better option would be to use Parallel.ForEach and set the number of concurrent threads to something *relatively* low to avoid consuming too many Db connections. As already mentioned you have to create 1 db connection per reader. It is *almost* always good practice to create connections when needed and then dispose of them as soon as you are done using them. Do not share them between methods, this is almost always a bad idea. — Igor, Sep 20 '17 at 20:08

Cee McSharpface · Answer 1 · 2017-09-20T21:49:42.407

1

Unless multiple active resultsets (MARS) is enabled, only one active SqlDataReader at a time is allowed per connection (more info here).
So you would have to open a new connection inside each task, instead of sharing cnxCRM across tasks.

Connections are pooled, your approach might work out if the number of tasks is not too big, some tens or dozens but not more, then you risk contention.

There is no guarantee that parallelization will solve performance issues, especially if it ends up in the same execution plan targeting the same table(s).

edited Sep 20 '17 at 21:49

answered Sep 20 '17 at 20:08

Cee McSharpface

8,493
3
36
77

Good im going to try creating the connections in every call, to see, and also compare the cost difference. – Sergio Cabrera Gari Sep 20 '17 at 21:01
dlatikay as you were explaining there is no performing improvement by using the tasks – Sergio Cabrera Gari Sep 20 '17 at 21:10

score 1 · Answer 2 · answered Sep 20 '17 at 22:03

In addition to @dlatikay's answer, consider that you have one round-trip to the database per task and that you're creating concurrency in the database on the same tables. You're likely to use less compute resources by telling SQL Server in one go what data you want and allow SQL Server to optimize the data retrieval strategy. This would involve modifying your sp_Dashboard_GetBudget to accept multiple companies at once.

You don't state how large your database is, but fifteen seconds feels like an awfully long time to retrieve some account information. Run your query in SSMS and have a look at the query plan. There's a decent chance that adding an appropriate index will drastically speed the query.

Eric J, Thank you and that is true, my database is not that large to be 15 seconds the query. I will work on optimizing the query and i will let you know. Thank you — Sergio Cabrera Gari, Sep 21 '17 at 12:37

ADODB Concurrence issue using task c#

2 Answers2