What's the best way to multithread?

Question

I've got a little problem with my application. I have a database editor that hangs up sometimes when I try to update the database file. Not every time, but pretty often, and every time it happens right before any changes are made to the database. I figured it's because of not using multithreading. I've only started learning programming recently though, and I'm kind of lost even after reading through a few multithreading explanations. Could someone explain to me how should I implement it in my specific example?

    private void adjustStatsButton_Click(object sender, EventArgs e)
    {
            ReadWrite.AdjustStats(winnerInput.Text, loserInput.Text);
            winnerInput.Text = "";
            loserInput.Text = "";
            Refresh(leaderboardBox);
    }

    public class ReadWrite
    {
        public static void AdjustStats(string winner, string loser)
        {
            SQLiteConnection dbConnection = new SQLiteConnection("Data Source = Leaderboards.sqlite; Version = 3");

        string sql = "SELECT * FROM leaderboard WHERE name='" + winner + "'";
        SQLiteCommand command = new SQLiteCommand(sql, dbConnection);
        dbConnection.Open();
        SQLiteDataReader reader = command.ExecuteReader();


        double wrating = Convert.ToDouble(reader["rating"]);
        int wmatches = Convert.ToInt32(reader["matches"]);
        int wwins = Convert.ToInt32(reader["wins"]);

        sql = "SELECT * FROM leaderboard WHERE name='" + loser + "'";
        command = new SQLiteCommand(sql, dbConnection);
        reader = command.ExecuteReader();

        double lrating = Convert.ToDouble(reader["rating"]);
        int lmatches = Convert.ToInt32(reader["matches"]);
        int lwins = Convert.ToInt32(reader["wins"]);
        int llosses = Convert.ToInt32(reader["losses"]);

        double RC = (1 - ((wrating - lrating) / 200)) * 8;
        if (RC < 0) RC *= -1;
        if (RC < 4) RC = 4;
        else if (RC > 12) RC = 12;

        wmatches++;
        wwins++;
        lmatches++;
        llosses++;
        wrating += RC;
        if (wrating < 0) wrating = 0;
        lrating -= RC;
        if (lrating < 0) lrating = 0;
        double wwinrate = Convert.ToDouble(wwins) / wmatches;
        double lwinrate = Convert.ToDouble(lwins) / lmatches;

        sql = "UPDATE leaderboard SET rating=" + wrating + ", matches=" + wmatches + ", wins=" + wwins + ", winrate=" + wwinrate + " WHERE name='" + winner + "'";
        command = new SQLiteCommand(sql, dbConnection);
        command.ExecuteNonQuery();

        sql = "UPDATE leaderboard SET rating=" + lrating + ", matches=" + lmatches + ", losses=" + llosses + ", winrate=" + lwinrate + " WHERE name='" + loser + "'";
        command = new SQLiteCommand(sql, dbConnection);
        command.ExecuteNonQuery();
        dbConnection.Close();
        }
    }

Side note, beware of sql injection with the parameter names `winner` and `loser`. — smoksnes, Aug 15 '16 at 06:05
First, the code you have provided doesn't seem to be super heavy. But it seems as you are doing it in the GUI-thread, which you should avoid. Try using `BackgroundWorker`, as mentioned here http://stackoverflow.com/questions/6365887/can-you-link-to-a-good-example-of-using-backgroundworker-without-placing-it-on-a — smoksnes, Aug 15 '16 at 06:12
@smoksnes thanks, I'm making this app for personal use only though so I wasn't worried about any possible security issues. — Draxuss, Aug 15 '16 at 08:11
@Ephraim `public static void Refresh(RichTextBox obj) { obj.Text = null; ReadWrite.dbLoad(obj); obj.SelectAll(); obj.SelectionAlignment = HorizontalAlignment.Center; obj.SelectionColor = Color.Black; obj.SelectionTabs = new int[] { 25, 20, 5, 5, 140 }; }` but the problem occurs before the database is even updated, hence I figured this wasn't relevant. — Draxuss, Aug 15 '16 at 08:14
As @smoksnes mentioned, you are running the first code in the UI thread. Use a background worker to execute `ReadWrite.AdjustStats(winnerInput.Text, loserInput.Text);`. Data access and UI updates should be run on separe threads. The UI thread should handle all UI update related codes or events. — jegtugado, Aug 15 '16 at 08:21
@Ephraim I did, but how can I "wait" for the `BackgroundWorker` to finish it's job before clearing the textboxes and calling `Refresh(leaderboardBox);` ? If I don't, it sends null values into the sql command now. — Draxuss, Aug 15 '16 at 08:32

score 0 · Answer 1 · answered Aug 15 '16 at 06:11

You just need to use async await keywords within your c# code and it will eventually lead Asynchronous calls to the DB I have made changes to your Code :

private async void adjustStatsButton_Click(object sender, EventArgs e)
    {
            await ReadWrite.AdjustStats(winnerInput.Text, loserInput.Text);
            winnerInput.Text = "";
            loserInput.Text = "";
            Refresh(leaderboardBox);
    }


    public class ReadWrite
        {
            public static Task AdjustStats(string winner, string loser)
            {
    return Task.Run(() => 
    {
       SQLiteConnection dbConnection = new SQLiteConnection("Data Source = Leaderboards.sqlite; Version = 3");


            string sql = "SELECT * FROM leaderboard WHERE name='" + winner + "'";
            SQLiteCommand command = new SQLiteCommand(sql, dbConnection);
            dbConnection.Open();
            SQLiteDataReader reader = await command.ExecuteReaderAsync();


            double wrating = Convert.ToDouble(reader["rating"]);
            int wmatches = Convert.ToInt32(reader["matches"]);
            int wwins = Convert.ToInt32(reader["wins"]);

            sql = "SELECT * FROM leaderboard WHERE name='" + loser + "'";
            command = new SQLiteCommand(sql, dbConnection);
            reader = await command.ExecuteReaderAsync();

            double lrating = Convert.ToDouble(reader["rating"]);
            int lmatches = Convert.ToInt32(reader["matches"]);
            int lwins = Convert.ToInt32(reader["wins"]);
            int llosses = Convert.ToInt32(reader["losses"]);

            double RC = (1 - ((wrating - lrating) / 200)) * 8;
            if (RC < 0) RC *= -1;
            if (RC < 4) RC = 4;
            else if (RC > 12) RC = 12;

            wmatches++;
            wwins++;
            lmatches++;
            llosses++;
            wrating += RC;
            if (wrating < 0) wrating = 0;
            lrating -= RC;
            if (lrating < 0) lrating = 0;
            double wwinrate = Convert.ToDouble(wwins) / wmatches;
            double lwinrate = Convert.ToDouble(lwins) / lmatches;

            sql = "UPDATE leaderboard SET rating=" + wrating + ", matches=" + wmatches + ", wins=" + wwins + ", winrate=" + wwinrate + " WHERE name='" + winner + "'";
            command = new SQLiteCommand(sql, dbConnection);
            await command.ExecuteNonQueryAsync();

            sql = "UPDATE leaderboard SET rating=" + lrating + ", matches=" + lmatches + ", losses=" + llosses + ", winrate=" + lwinrate + " WHERE name='" + loser + "'";
            command = new SQLiteCommand(sql, dbConnection);
            await command.ExecuteNonQueryAsync();
            dbConnection.Close();
            }
});

        }

This is a single thread/Task you used. You could do the winning and the losing on seperates threads with an await of the third thread that does the rest of the code i.e from `double wrating = Convert.ToDouble` — Neil, Aug 15 '16 at 06:18
I did not used that approach because one thread would be enough for it in order to make your UI responsive for the user — Lakhtey, Aug 15 '16 at 06:27
@Lakhtey The code for `public static Task AdjustStats(string winner, string loser)` is showing errors with the "await" parts, telling me to change `return Task.Run(() =>` to `return Task.Run(async() =>` , but when I do that it says in turn that it can't convert `System.Data.Common.DbDataReader` to `System.Data.SQLite.SQLiteDataReader` because for some reason it tries to execute the DbDataReader version of `ExecuteReaderAsync();` ? I'm not sure I understand what's happening here. — Draxuss, Aug 15 '16 at 08:15

score 0 · Accepted Answer · edited May 23 '17 at 12:08

The problem is that you are running the query in the UI thread. Here's an example for using the BackgroundWorker, heavily inspired from this example, and using Arguments. This way it will be running in a separate thread and it will not lock the GUI.

// Class for passing arguments
public class BqArguments
{
    public string Winner {get;set}
    public string Loser {get;set}
}

And your implementation:

BackgroundWorker _bw; // You need to initialize this somewhere.

private void adjustStatsButton_Click(object sender, EventArgs e)
{
    // Maybe this should be initialized in ctor. But for this example we do it here...
    _bw = new BackgroundWorker();
    var arguments = new BqArguments
    {
       Winner = winnerInput.Text,
       Loser = loserInput.Text
    }
    _bw.DoWork += bw_DoWork;
    _bw.RunWorkerCompleted += bw_RunWorkerCompleted;
    _bw.RunWorkerAsync(arguments);    
}

private void bw_DoWork (object sender, DoWorkEventArgs e)
{
    // Run your query in the background.
    var arguments = e.Argument as BqArguments;
    ReadWrite.AdjustStats(arguments.Winner, arguments.Loser);
}

private void bw_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
{
    // All done. Let's update the GUI.
    winnerInput.Text = "";
    loserInput.Text = "";
    Refresh(leaderboardBox);
}

Works great, but I had to change `e.Arguments` in `bw_DoWork` to `e.Argument` because apparently "'DoWorkEventArgs' does not contain a definition for 'Arguments'" — Draxuss, Aug 15 '16 at 08:53
yeah, I spent a bit of time trying to figure out why wasn't it working, but it's all good now. Thanks! — Draxuss, Aug 15 '16 at 09:02

score 0 · Answer 3 · edited May 23 '17 at 10:28

0

The core of your problem is that you're calling blocking (synchronous) database calls on the UI thread. This blocks your UI thread while it's waiting for the database, instead of keeping it nicely responsive.

In the general case, since these are I/O-based operations, you should be able to make them naturally asynchronous, as per Lakhtey's answer. However, SQLite does not support actual asynchronous operations. :(

So, in this case, your best bet is to just wrap up the database work into a background thread. Note that using Task.Run is far superior to BackgroundWorker:

private async void adjustStatsButton_Click(object sender, EventArgs e)
{
  await Task.Run(() => ReadWrite.AdjustStats(winnerInput.Text, loserInput.Text));
  winnerInput.Text = "";
  loserInput.Text = "";
  Refresh(leaderboardBox);
}

edited May 23 '17 at 10:28

Community

1
1

answered Aug 15 '16 at 13:53

Stephen Cleary

437,863
77
675
810

Thanks Stephen. What would be the best choice here when you say superior? – Zuzlx Aug 15 '16 at 17:33
@Zuzlx: For SQLite, I would use `Task.Run`. – Stephen Cleary Aug 15 '16 at 18:12
@StephenCleary so the only thing I'd need to do is add the `await Task.Run(() => [..] )` and it will automatically handle it on a different thread? Won't it cause a problem with the `winnerInput.Text` being set to `null`? – Draxuss Aug 16 '16 at 07:55
@Draxuss: I don't see where `winnerInput.Text` is set to `null`. It is set to an empty string *outside* the `Task.Run`, and that's fine. – Stephen Cleary Aug 16 '16 at 12:31
@StephenCleary yeah that's what I meant, sorry. What I'm afraid of happening basically is it changing to an empty string before the task finishes since it's asynchronous. – Draxuss Aug 16 '16 at 14:30
@Draxuss: `await` is an "asynchronous wait". So `Text` will be set to an empty string *after* `AdjustStats` completes. For more info on `async`, see my [`async` intro blog post](http://blog.stephencleary.com/2012/02/async-and-await.html). – Stephen Cleary Aug 16 '16 at 16:56

What's the best way to multithread?

3 Answers3