Is this correct way to setup DB Infrastructure with repository pattern and Dapper

Question

So, I am using Repository pattern with Dapper and ADO.NET. I have my based DB infrastructure setup as follows:

public class ConnectionFactory: IConnectionFactory
{
 private readonly string dbConnectionString = Convert.ToString(ConfigurationManager.ConnectionStrings["MyDBConn"].ConnectionString);
 private MySqlConnection _dbConnection;
 public IDbConnection GetDBConnection
 {
 get
 {
 if(string.IsNullOrEmpty(dbConnectionString))
 {
 throw new ArgumentNullException("DB Connection string received a null argument!");
 }
 if (_dbConnection == null)
 {
 _dbConnection = new MySqlConnection(dbConnectionString);
 }
 if (_dbConnection.State != ConnectionState.Open)
 {
 _dbConnection.Open();
 }
 return _dbConnection;
 }
 }
 public void CloseDBConnection()
 {
 if (_dbConnection != null && _dbConnection.State == ConnectionState.Open)
 {
 _dbConnection.Close();
 }
 }
}

And I have a Repository class that uses this infrastructure:

public async Task<IEnumerable<UserInformation>> GetUserInformation()
{
 IEnumerable<UserInformation> list;
 string querystring = "SELECT * FROM users WHERE valid=1;";
 try
 {
 list = await SqlMapper.QueryAsync<UserInformation>(_connectionFactory.GetDBConnection, querystring, param, commandType: CommandType.Text);
 }
 finally
 {
 _connectionFactory.CloseDBConnection();
 }
 return list;
}

For reading my IReader and converting to a dataset, I do the following:

public async Task<DataSet> GetOrderDetails(int oid)
{
 DataSet dataset = new DataSet();
 string querystring = "select * from orders WHERE OrderId=@oid";
 DynamicParameters param = new DynamicParameters();
 param.Add("@oid", oid);
 using (var list = await SqlMapper.ExecuteReaderAsync(_connectionFactory.GetDBConnection, querystring, param, commandType: CommandType.Text))
 {
 dataset = helper.ConvertDataReaderToDataSet(list);
 }
 return dataset;
}

Here ConvertDataReaderToDataSet is a method that only converts the IReader to a Dataset.

The problem that I having is that I am getting the following error after a while (from logs):

2/10/2021 12:37:44 AM
Type: Exception
Error Message: error connecting: Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.
Method Name: GetOrderDetails

I have researched this error and it suggests a memory leak. Can you guys help me identify what is the problem and review my DB infrastructure setup?

Edit: After a discussion, I have changed the implementation to this:

public async Task<IEnumerable<UserInformation>> GetUserInformation()
{
 IEnumerable<UserInformation> list;
 string querystring = "SELECT * FROM users WHERE valid=1;";
 using (var conn = new MySqlConnection(dbConnectionString))
 {
 list = await conn.QueryAsync<UserInformation>(querystring, commandType: CommandType.Text);
 }
 return list;
}

• pacmaninbw: Are you still getting the error message after the code edit? For future reference code review is for reviewing working code, a question containing code that is not working as expected should be possibly posted on stack overflow. If the code is now working as expected please remove the code that wasn't working and the error message.
• Rahul Sharma: @pacmaninbw This code is working. The question is about it's performance and better techniques on how it can be handled.
• Jesse C. Slicer: _dbConnectionString is not a string. It's a connection.
• Rahul Sharma: @JesseC.Slicer Yes, I have made the required edit not to mislead readers.
• 1201ProgramAlarm: If you've changed GetUserInformation, and the broken version is not discussed in an answer, there is no reason to keep the broken version in the question.

Answer 1

Quick remarks:

• Why do you convert a string to a string: Convert.ToString(ConfigurationManager.ConnectionStrings["MyDBConn"].ConnectionString);? It's even called a ConnectionString.

• Why do you use Dapper and then don't use it properly? Why return a DataSet? Why use SqlMapper and DynamicParameters? Why make life so hard for yourself when Dapper is making things so easy? Look at the homepage of Dapper Tutorial and Documentation:

   using (var conn = new SqlConnection(connectionString))
   {
   var invoices = conn.Query<Invoice>(sql);
   }
  

  That is all you need. No endless checking of the state of your connection, no opening and closing it yourself, no try...finally blocks which completely ignore any exception that might occur. Write the correct query and let Dapper map the results to your class.

• Rahul Sharma: Thanks, I will make a note of this. So basically you are saying the current DB infrastructure has problems? If you could point where, that would be great.
• BCdotWEB: @RahulSharma My main problem is that you seem to apply an outdated ADO.NET style code to Dapper. Whereas Dapper should (IMHO) be used is a quite simple way, as per the example. The whole GetDBConnection / CloseDBConnection is pointless, just use using (which you actually do in one of your methods!). Checking whether a connection string isn't null on every call is pointless. Should you try...catch possible exceptions when executing a query? Possibly, but that depends on your case.
• BCdotWEB: Also, encapsulating a db connection in a using statement also properly disposes of the connection, which most of your code doesn't. See stackoverflow.com/a/17168919/648075 .
• BCdotWEB: @RahulSharma Please stop reposting your comment. Whether your code now works is something only you can tell. I don't know how SqlMapper works and whether it properly disposes of a connection (but I assume it doesn't), so my advice should be applied everywhere you use a db connection. But you wouldn't have those issues if you'd use Dapper the "proper" (i.e. simplest) way (e.g. I have never passed the commandType parameter in Dapper calls).