current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

I've written a class to make async SQL calls and it appears to work like a charm! But I'm a bit concerned about what it means to send a lot of queries to the server and then aborting them by throwing an abort exception on the calling thread. Is this a problem?

The code also launches a lot of threads. Is there any downside to this? Should I use ThreadPool instead, and if so, how?

I'm using .NET 4.

//Class for asynchronous SQL calls
public class SqlAsync
{
    private Thread LastAction { get; set; }
    private string ConnectionString { get; set; }
    public int Timeout { get; set; }

    public SqlAsync(string connstring)
    {
        this.ConnectionString = connstring;
        this.Timeout = 30;
    }

    public void AsyncSqlCall<T>(string sp, Action<T> Callback, Func<SqlDataReader, T> HandleResult, Dictionary<string, object> Params = null)
    {
        TryKillThread(this.LastAction);

        this.LastAction = new Thread(() =>
        {
            T returnobj;
            using (SqlConnection conn = new SqlConnection(this.ConnectionString))
            using (SqlCommand cmd = new SqlCommand(sp, conn))
            {
                cmd.CommandTimeout = this.Timeout;
                cmd.CommandType = CommandType.StoredProcedure;
                if (Params != null)
                {
                    foreach (KeyValuePair<string, object> kvp in Params)
                    {
                        cmd.Parameters.AddWithValue(kvp.Key, kvp.Value ?? DBNull.Value);
                    }
                }
                conn.Open();
                using (SqlDataReader rdr = cmd.ExecuteReader())
                {
                    returnobj = HandleResult(rdr);
                    rdr.Close();
                }
                conn.Close();
            }
            new Thread(() => Callback(returnobj)).Start();
        });
        this.LastAction.Start();
    }

    private void TryKillThread(Thread thread)
    {
        if (thread != null && thread.IsAlive)
        {
            try
            {
                thread.Abort();
            }
            catch (ThreadStateException) 
            { /*If thread ends between IsAlive check and Abort call, then just ignore*/ }
        }
    }
}

This is an example of how I use the class

public class Example : Form
{
    DbExample DbEx = new DbExample("");
    public void tbExample_TextChanged(object sender, EventArgs e)
    {
        DbEx.BeginGetSomething(tbExample.Text, BindDgvSomething);
    }
    private void BindDgvSomething(DataTable dt)
    {
        dgvSomething.Invoke(new Action(() => dgvSomething.DataSource = dt));
    }
}

public class DbExample
{
    private SqlAsync SqlAsync;
    private string Connstr;

    public DbExample(string connstr)
    {
        this.Connstr = connstr;
        this.SqlAsync = new SqlAsync(this.Connstr);
    }
    private DataTable LoadReader(SqlDataReader rdr)
    {
        DataTable dt = new DataTable();
        dt.Load(rdr);
        return dt;
    }
    public void BeginGetSomething(string name, Action<DataTable> Callback)
    {
        Dictionary<string, object> Params = new Dictionary<string, object>() 
        {
            { "@Name", name }
        };

        SqlAsync.AsyncSqlCall("spGetSomething", Callback, LoadReader, Params);
    }
}
share|improve this question

1 Answer 1

up vote 4 down vote accepted
  • Your code does not follow naming conventions (private fields should be camelCased, and usually have an underscore prefix; parameters should be camelCased)
  • there is no need in private properties, use fields instead (ConnectionString)
  • It is not an asynchronous version of SQL access, it is multithreaded access. The difference is that you don't necessarily need to span new threads (and you should not, in fact) to make your code asynchronous.
  • Stopping a thread using Thread.Abort is discouraged, and in general is very dangerous as it may lead to leaked resources and a lot of other side effects. It was explained in a lot of places, e.g. How To Stop a Thread in .NET (and Why Thread.Abort is Evil)
  • Killing a previous SQL action upon running the new one is the best way to get random issues all over your application
  • You don't provide any means to control the execution of asynchronous request, so calling code has no idea whether SQL finished execution
  • And finally, AsyncSqlCall<T> method should not have Action<T> Callback parameter as it is a continuation action, you should return Task<T> instead (so that caller can add a continuation)

I don't like custom ORMs and would suggest you to use Entity Framework or NHibernate as a data layer, but as an exercise here are the fixes I would apply (.NET 4.5):

public class SqlAsync
{
    private readonly string _connectionString;
    public int Timeout { get; set; }

    public SqlAsync(string connstring)
    {
        _connectionString = connstring;
        Timeout = 30;
    }

    public Task<T> AsyncSqlCall<T>(string sp, Func<SqlDataReader, Task<T>> handleResult, Dictionary<string, object> parameters = null)
    {
        return AsyncSqlCall<T>(sp, (reader, token) => handleResult(reader), CancellationToken.None, parameters);
    }

    public async Task<T> AsyncSqlCall<T>(string sp, Func<SqlDataReader, CancellationToken, Task<T>> handleResult, CancellationToken cancellationToken, Dictionary<string, object> parameters = null)
    {
        using (SqlConnection conn = new SqlConnection(_connectionString))
        using (SqlCommand cmd = new SqlCommand(sp, conn))
        {
            cmd.CommandTimeout = Timeout;
            cmd.CommandType = CommandType.StoredProcedure;

            if (parameters != null)
            {
                foreach (KeyValuePair<string, object> kvp in parameters)
                    cmd.Parameters.AddWithValue(kvp.Key, kvp.Value ?? DBNull.Value);
            }

            await conn.OpenAsync(cancellationToken);

            using (SqlDataReader rdr = await cmd.ExecuteReaderAsync(cancellationToken))
                return await handleResult(rdr, cancellationToken);
        }
    }
}
share|improve this answer
    
Hi, thanks for your response! I realize await/async is the more modern approach to this problem but I'm not using .NET 4.5, I should've specified that in my question. How else would you cancel an ongoing SQL query if not calling Thread.Abort? Since all disposable objects are wrapped in using statements, where could the leak occur? In the internal .NET sql classes? Notice the SqlAsync class is not static and the field in Example class is private so the call does not interfer with calls from any other class. How do you mean I could get random issues all over the application? –  Mcb2000 Feb 20 '13 at 9:04
    
@Mcb2000 If you read the article carefully you could have noticed the link to Why Thread.Abort is Evil where example of resource leak is shown for using statement. Example class may decide to issue several asynchronous SQL requests and all of them but last would be aborted –  almaz Feb 20 '13 at 9:13
    
Ah, that's a very good point about exception being raised during disposal in finally. Though ugly, I could replace the using with a try-finally{try-catch} block. That needs to be thought through. Having all but the most recent SQL call aborted is the behaviour I want from this class. In the example I provided it is used to filter a datagrid based on textbox input. If the user writes 6 characters, I don't need all 6 queries to finish executing and returning the result of each one. Is there any danger of memory leak with starting this many threads, given they all finish at an acceptable point? –  Mcb2000 Feb 20 '13 at 9:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.