5
\$\begingroup\$

Since I'm venturing more and more into the multithreading, I now need to think about how to protect my precious OdbcConnection from breaking at random times.

The project specifications:

  • .NET-2.0
  • ODBC-Connection via MySQL-Connector 3.51 to MySQL 5.0

The main structure of my project in question looks like this:

+ Assembly
+--+> Namespace1
   +--> Data-Layer
+--+> Namespace2
   +--> Data-Layer
...

There's also an abstraction layer in place with interfaces and implementations, but I think that doesn't matter, because my question is about the ODBC implementation. They do also implement IDisposable.

This means I'm passing around an OdbcConnection to different classes and create these Data classes if asked to, like this:

public class MainDataSource
{
    private OdbcConnection _conn = <Yes, It's initialized>;

    public Namespace1.Data CreateNamespace1Data()
    {
        return new Namespace1.Data(_conn);
    }

    public Namespace2.Data CreateNamespace2Data()
    {
        return new Namespace2.Data(_conn);
    }
}

The Data classes itself are containing methods to fetch objects from the database. The only interesting thing here is that every Data class is holding its very own OdbcCommand objects, none of these are shared in any way. The only thing they have in common is the source OdbcConnection.

Since some of these classes (like the Setting class) might be called from multiple threads at the same time, I need to think about the best way to lock and protect these. My first idea was to lock the whole OdbcConnection:

public class Data
{
    private OdbcConnection _conn;
    private OdbcCommand _cmd;

    public Data(OdbcConnection conn)
    {
        _conn = conn;

        _cmd = _conn.CreateCommand();
        _cmd.CommandText = <FromResource>;
        _cmd.Parametersblabla;
    }

    public MyDataObject GetObjects(String searchterm)
    {
        lock(_conn)
        {
            _cmd.Parameters[0].Value = searchterm;
            using(OdbcDataReader reader = _cmd.ExecuteReader())
            {
                // Do stuff here and return MyDataObject
            }
        }
    }
}

This seems a little bit too strict to me, so I also thought about only locking the OdbcCommand:

public class Data
{
    private OdbcCommand _cmd;

    public Data(OdbcConnection conn)
    {
        _cmd = conn.CreateCommand();
        _cmd.CommandText = <FromResource>;
        _cmd.Parametersblabla;
    }

    public MyDataObject GetObjects(String searchterm)
    {
        lock(_cmd)
        {
            _cmd.Parameters[0].Value = searchterm;
            using(OdbcDataReader reader = _cmd.ExecuteReader())
            {
                // Do stuff here and return MyDataObject
            }
        }
    }
}

But I still feel like I've missed something very important here. Do I have other options? Did I miss something important?

\$\endgroup\$
6
\$\begingroup\$

The OdbcConnection is not thread-safe in any way, therefor it needs a locking mechanism.

My observation is that the OdbcConnection does send it's received commands down into the Connector. If this is happening in a multi-threaded environment, the sequence of commands is interrupted and you'll receive an exception with this or a similar message:

ERROR [HY000] [MySQL][ODBC 3.51 Driver]Commands out of sync; you can't run this command now

The driver is confused because the sequence was interrupted. This can not only happen on a OdbcCommand level, but on a OdbcConnection level. The OdbcConnection needs to be locked.

An additional note is that you should lock the OdbcConnection the moment you start to work with a command:

_cmd.Parameters[0].Value = argument;
lock(_conn)
{
    _cmd.ExecuteNonQuery();
}

This is bad, because it could happen in a multi-threaded environment that the parameter gets overridden by another thread before ExecuteNonQuery can be called.

lock(_conn)
{
    _cmd.Parameters[0].Value = argument;
    _cmd.ExecuteNonQuery();
}

This keeps the execution as one atomic operation which can't be interrupted.

Further investigations revealed that the MySQL Odbc-Connector has a locking mechanism...but only on a statement level:

MySQL ODBC Connector, 5.1.9, execute.c

 39:    SQLRETURN do_query(STMT FAR *stmt,char *query, SQLULEN query_length)
 40:    {
 ...
 55:        MYLOG_QUERY(stmt, query);
 56:        pthread_mutex_lock(&stmt->dbc->lock);
 ...
 96:        exit:
 97:        pthread_mutex_unlock(&stmt->dbc->lock);
 ...
114:   }

And to make it clear: If you have a non-thread-safe connection, lock the whole connection.

And to make it further clear, using an OdbcDataReader counts as using the connection. As long as you work with an OdbcDataReader, you should keep the lock on the connection.

This includes disposing of the resources, f.e. a DataReader. You should always use using inside the locked parts to make sure that the resources are freed again, otherwise it could happen that a DataReader lingers on and is later destroyed by the Garbage Collector and a rather inconvenient moment, like this:

Thread  Action
     1  Lock Connection
     1  Create Statement
     2  Lock Connection -> Wait
     1  Prepare Statement
     1  Execute Statement
     1  Get Reader
     1  Use Reader
     1  Unlock Connection
     2  Lock Connection
     2  Prepare Statement
     2  Execute Statement
    GC  Destroy Statement from thread #1
    GC  Destroy Reader from thread #1

In code it would like this:

lock (connection)
{
    IDbCommand command = connection.CreateCommand();
    command.CommandText = "SQL";
    IDataReader reader = command.ExecuteReader();
    // Do something the reader
    return valuesFromTheReader;
}

This is bad as it allows resources to linger on and the GC will later destroy them, with possible disastrous results. My tests revealed that from the exception

Attempted to read or write protected memory.

to a simple deadlock of the application on an arbitrary line1 everything is possible if you fail to correctly synchronize database access.

The best pattern is to always destroy everything while in inside the lock:

lock (connection)
{
    using (IDbCommand command = connection.CreateCommand())
    {
        command.CommandText = "SQL";
        using (IDataReader reader = command.ExecuteReader())
        {
            // Do something the reader
            return valuesFromTheReader;
        }
    }
}

1: I'm not kidding, I could reliable reproduce a hang on the following instruction inside the System.Data.Odbc.OdbcDataReader.NextResult(bool, bool)

IL_0099: ldarg.1
00000099 8B F8 mov edi,eax

when I did not properly dispose of a DataReader.

\$\endgroup\$
1
\$\begingroup\$

Is there any reason why you need to have only one connection? I would have each method that needs to interact with the database open its own connection and dispose it when it is finished. If you ensure connection pooling is enabled it will take care of reusing connections so you won't overload the database server with connection requests.

I'd also create a static method that creates and opens the connection, it would take care of getting your connection string and would simply return an open connection. Calling code would then just need to start a using block calling this function.

This also has the advantage that multiple threads can be performing data access at the same time, otherwise youmay be limiting the benefit of your multithreading.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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