0

I am developing an app in WPF. I am trying to fetch email, extract data, save it in DB and work with it in DataGrid. To avoid saving duplicates, I have made a method for that. My problem is, that I don't know why my dataReader is closed? If I am using it in another .xaml class it works properly, so what is wrong here? I have used using-statement, where I thought was the problem, but it seems that it is not.

DataReader method:

public IEnumerable<Problem> GetAll()
{
    using (SqlConnection conn = DatabaseSingleton.GetInstance())
    {
        //conn.Open();
        using (SqlCommand command = new SqlCommand("SELECT * FROM Problem", conn))
        {
            using (SqlDataReader reader = command.ExecuteReader())
            {
                while (reader.Read())
                {
                    Problem problem = new Problem
                    {
                        Id = reader.GetInt32(0),
                        NameOfAlert = reader.GetString(1),
                        Value = Enum.Parse<Value>(reader[2].ToString()),
                        Result = Enum.Parse<Result>(reader[3].ToString()),
                        Message_Id = reader.GetString(4)
                    };
                    yield return problem;
                }
            }
        }
        //conn.Close();
    }
}

Method for fetching emails:

public static void MailKitLib(EmailParser emailParser)
{
    bool help = true;
    
    do
    {
        using (var client = new ImapClient())
        {
            using (var cancel = new System.Threading.CancellationTokenSource())
            {
                client.Connect(emailParser.ServerName, emailParser.Port, emailParser.IsSSLuse, cancel.Token);
                client.Authenticate(emailParser.Username, emailParser.Password, cancel.Token);
    
                var inbox = client.Inbox;
                inbox.Open(FolderAccess.ReadOnly, cancel.Token);
    
                //Here I am saving only originals
                for (int i = 0; i < inbox.Count; i++)
                {
                    var message = inbox.GetMessage(i, cancel.Token);
                    GetBodyText = message.TextBody;
                    Problem problem = new Problem(message.MessageId);
                    if (!dAOProblem.GetAll().Any(x => x.Message_Id.Equals(problem.Message_Id)))
                    {
                        dAOProblem.Save(problem);
                        Alert alert = new Alert(message.MessageId, message.Date.DateTime, message.From.ToString(), 1, problem.Id);
                        if (!dAOAlert.GetAll().Any(x => x.Id_MimeMessage.Equals(alert.Id_MimeMessage)))
                        {
                            dAOAlert.Save(alert);
                        }
                    }
                }
                //client.Disconnect(true, cancel.Token);
            }
        }
    } while (help != false);
}

UPDATE: This is exactly what I have tried. But there is also a problem that connection was not closed.

Palle Due
  • 5,929
  • 4
  • 17
  • 32
jirina
  • 41
  • 6
  • Where EXACTLY is a data reader closed that you don't think it should be. Please provide specific details. – John Aug 15 '22 at 07:38
  • 2
    `DatabaseSingleton.GetInstance()` Why you use singleton pattern on your SqlConnection? That does not help at all but can hurt you much, in case of a multi threading app. The "best" what it could do is to reduce performance, because [sql connection pooling](https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sql-server-connection-pooling) needs to create a new physical connection whener you want to open one. Instead, always create a new instance and use `using (var conn = GetConnection()) ...`. It's important to close the connection to make it reusable for the pool. – Tim Schmelter Aug 15 '22 at 07:41
  • @John in first and second `if-statement` it throws the exception – jirina Aug 15 '22 at 07:42
  • @DiplomacyNotWar Yes I have. How I said I also used using-statement like Tim said before, but without results – jirina Aug 15 '22 at 08:00

2 Answers2

2

The problem was with DatabaseSingleton.GetInstance(), which supported connection-pooling

 public IEnumerable<Problem> GetAll()
        {
            string connString = "Data Source=;Initial Catalog=;User ID=;Password=";
            using (SqlConnection conn = new SqlConnection(connString))
            {
                conn.Open();
                using (SqlCommand command = new SqlCommand("SELECT * FROM Problem", conn))
                {
                    using (SqlDataReader reader = command.ExecuteReader())
                    {
                        while (reader.Read())
                        {
                            Problem problem = new Problem
                            {
                                Id = reader.GetInt32(0),
                                NameOfAlert = reader.GetString(1),
                                Value = Enum.Parse<Value>(reader[2].ToString()),
                                Result = Enum.Parse<Result>(reader[3].ToString()),
                                Message_Id = reader.GetString(4)

                            };
                            yield return problem;
                        }
                    }
                conn.Close();
                }
            }
       
jirina
  • 41
  • 6
1

First problem: Get rid of your database singleton. Connection pooling will take care of it. There is no performance gain in you reusing conenctions, let the system handle it.

Second problem: yield and using aren't good friends, because using might dispose of the instance before yielding is done. You might be able to solve it by dropping the using on the connection and make the reader close the connection when it's done by CommandBehavior.CloseConnection. Otherwise look at the solutions here: yield return statement inside a using() { } block Disposes before executing

Problem 2 was a misunderstanding of mine. Please see @CharlieFace's comment.

public IEnumerable<Problem> GetAll()
{
    var conn = new SqlConnection(*connectionString*);
    conn.Open();
    using (SqlCommand command = new SqlCommand("SELECT * FROM Problem", conn))
    {
        using (SqlDataReader reader = command.ExecuteReader(CommandBehavior.CloseConnection))
        {
            while (reader.Read())
            {
                Problem problem = new Problem
                {
                    Id = reader.GetInt32(0),
                    NameOfAlert = reader.GetString(1),
                    Value = Enum.Parse<Value>(reader[2].ToString()),
                    Result = Enum.Parse<Result>(reader[3].ToString()),
                    Message_Id = reader.GetString(4)
                };
                yield return problem;
            }
        }
    }
}
Palle Due
  • 5,929
  • 4
  • 17
  • 32
  • Yeah, thanks for your answer. You was faster than me with posting right answer. But finding that `yield return` and `using` is not good idea is great info. for me. Thanks. – jirina Aug 15 '22 at 09:29
  • 1
    "`yield` and `using` aren't good friends, because `using` might dispose of the instance before yielding is done." that's not true. It's sorted out by the compiler lifting the implicit `finally` of the `using` into a `Dispose()` function on the iterator. The issue in the link you posted does not have a `yield return` in the outer function and the solution was to add one. See also https://stackoverflow.com/a/58449809/14868997 – Charlieface Aug 15 '22 at 11:27
  • @Charlieface: Sorry for my misunderstanding. I'll edit the answer. – Palle Due Aug 15 '22 at 11:31