0

I am trying to have a clean excel break once I load a user's settings file. I am running into something I can't figure out other than it has something to do with the dictionaries. If I comment out one (or both) of the Dictionary filling brackets, the settings file loads and then releases, but if both are running, the excel app won't release. Am I also tying excel to the dictionaries by sourcing the data from them?

I am sure there are other ways to create global dictionaries, but this is the only way I am confidant with at the moment, but I am willing to learn if something better is out there.

The "dictionary filling code" is the for loops with i & j:

for (int i = 0; i < lastRow - 1; i++)
    {
        string key = settingsSheet.Range["B" + (i + 2)].Value; 
        string value = settingsSheet.Range["A" + (i + 2)].Value; 
        DictionaryLoad.DIC.Add(key, value);
    }

here is the full code:

public Form1()
    {
        InitializeComponent();
        txtFileNamePreface.Enabled = false;
        string fileName = "F:\\Shared\\Projects\\State Assoc Clients\\Data Management\\Download Site\\KeyStats Download Statistics\\Naming Conventions.xls";
        LoadProductName(fileName);
    }

    public static class DictionaryLoad
    {
        public static IDictionary<string, string> DIC;
        public static IDictionary<string, string> DIC2;
        static DictionaryLoad()
        {
            DIC = new Dictionary<string, string>();
            DIC2 = new Dictionary<string, string>();
        }
    }

    private void LoadProductName(string fileName)
    {
        //starting up and defining the Excel references
        Excel.Application excelApp = new Excel.Application(); //excel open here
        Excel.Workbook settingsBook = null;
        Excel.Worksheet settingsSheet = null;
        excelApp.Visible = false;
        excelApp.DisplayAlerts = false;

        settingsBook = excelApp.Workbooks.Open(fileName);
        settingsSheet = settingsBook.Sheets["NamingConventions"]; 

        int lastRow = findFirstBlankRow(settingsSheet, "A1") - 1;
        fillComboBox(cbProductType, lastRow, settingsSheet, "A"); 
        fillComboBox(cbYear, lastRow, settingsSheet, "D");

        int lastRow2 = findFirstBlankRow(settingsSheet, "E1");
        fillComboBox(cbRule, lastRow2, settingsSheet, "E");

        for (int i = 0; i < lastRow - 1; i++)
        {
            string key = settingsSheet.Range["B" + (i + 2)].Value; 
            string value = settingsSheet.Range["A" + (i + 2)].Value; 
            DictionaryLoad.DIC.Add(key, value);
        }

        cbProductName.Items.Clear();
        foreach (KeyValuePair<string, string> entry in DictionaryLoad.DIC)
        {
            if (entry.Value == cbProductType.Text)
            { cbProductName.Items.Add(entry.Key); }
        }
        try { cbProductName.SelectedIndex = 0; }
        catch { }

        for (int j = 0; j < lastRow - 1; j++)
        {
            string key = settingsSheet.Range["B" + (j + 2)].Value; 
            string value = settingsSheet.Range["C" + (j + 2)].Value;
            DictionaryLoad.DIC2.Add(key, value);
        }

        cbRule.SelectedIndex = 0;
        cbYear.Text = DateTime.Now.Year.ToString();
        cbQuarter.SelectedIndex = 0;
        cbMonth.Text = DateTime.Now.ToString("MMMM");
        cbProductType.SelectedIndex = 0;
        string workBookName = excelApp.ActiveWorkbook.FullName;
        txtOutputFolder.Text = Path.GetDirectoryName(workBookName);

        settingsBook.Close();
        excelApp.Quit();
        appCleanup(excelApp);
        appCleanup(settingsBook);
        appCleanup(settingsSheet);
        garbageCleanup();
        Application.Exit();
    }
   public void appCleanup(object application1, object application2 = null, object application3 = null)
    {
        Marshal.ReleaseComObject(application1);
        application1 = null;
    }

    public void garbageCleanup()
    {
        GC.Collect();
        GC.WaitForPendingFinalizers();
    }
Govert
  • 16,387
  • 4
  • 60
  • 70
Darw1n34
  • 312
  • 7
  • 24
  • What is the appCleanup method code? – Glen Thomas Sep 14 '15 at 19:23
  • And what do you mean by "the Dictionary filling brackets" – Glen Thomas Sep 14 '15 at 19:25
  • @GlenThomas I am sorry, I will add that in an edit - added info for both comments – Darw1n34 Sep 14 '15 at 19:25
  • @Darw1n34 I think you should release the COM objects in an other order: sheet first, then book, then app. – silkfire Sep 14 '15 at 19:29
  • @silkfire Thanks, I just slapped myself in the forehead and tried but that still leaves Excel App open. – Darw1n34 Sep 14 '15 at 19:38
  • I think maybe you should do settingsSheet.Range["B" + (i + 2)].Value.Copy(), because otherwise your dictionaries are holding a reference to string objects linked to the Excel objects. This will create a new instance of the strings. – Glen Thomas Sep 14 '15 at 19:40
  • @GlenThomas Thank you for looking, but i get the following error: The name 'Copy' is bound to a method and cannot be used like a property – Darw1n34 Sep 14 '15 at 19:42
  • 1
    You didn't add the () brackets? Copy is a method – Glen Thomas Sep 14 '15 at 19:44
  • @Darw1n34 Also I think `Application.Exit()` is unnecessary after releasing the objects. Try using `.ToString()` on the Value objects? – silkfire Sep 14 '15 at 19:45
  • @GlenThomas I did it both ways, that tells me "No overload methods for "Copy" take zero arguments" I will chase this one down further thought it doesn't explain why if I only comment out one, it will work. – Darw1n34 Sep 14 '15 at 19:48
  • @silkfire that didn't work, I went right to that after GlenThomas' suggestions. – Darw1n34 Sep 14 '15 at 19:48
  • @Darw1n34 Sorry, I forgot the correct usage of the copy method. I'll post the code as an answer so its easier to read. If it doesn't work leave a comment and I'll delete it. – Glen Thomas Sep 14 '15 at 19:51
  • I think application1 = null; in the appcleanup method will not do what you expect because you are not using the ref keyword – Glen Thomas Sep 14 '15 at 20:13
  • Please see http://stackoverflow.com/questions/31524103/excel-process-will-not-terminate-with-excel-introp/31526669#31526669 – Sorceri Sep 14 '15 at 20:22

2 Answers2

2
  1. You never need to call Marshal.ReleaseComObject in this context. The runtime is perfectly able to keep track of COM objects and release them when they are no longer referenced. Calling Marshal.ReleaseComObject is a confusing anti-pattern that sadly even some Microsoft documentation mistakenly suggests.

    In your case, this means those calls in appCleanup should be removed, though you might still have to set application1 = null to clear the reference - your code does not should how this variable gets used.

  2. You should call the garbage collector routine twice - you might have cases where the references form a cycle, and the first GC call will break the cycle, but the COM objects might only get properly released on the second call.

  3. Finally, you have to be careful with this kind of code in debug builds. References in a method are artificially kept alive until the end of the method so that they will still be accessible in the debugger. This means your local excelApp variable won't be cleaned up by calling the GC inside that method. To avoid this issue, you might follow a pattern like this:

    public void LoadProductNameAndCleanup(string fileName)
    {
        LoadProductName(fileName);
        garbageCleanup();
        garbageCleanup();
    }
    
Govert
  • 16,387
  • 4
  • 60
  • 70
  • 1
    That did it. Thank you. You were correct as well, I took the use or the Marshal.ReleaseComObject from MSDN documentary on the issue. – Darw1n34 Sep 14 '15 at 20:38
1

I think maybe you should use string.Copy, because otherwise your dictionaries are holding a reference to string objects linked to the Excel objects. This will create a new instances of the string objects:

for (int i = 0; i < lastRow - 1; i++)
{
    string key = string.Copy(settingsSheet.Range["B" + (i + 2)].Value); 
    string value = string.Copy(settingsSheet.Range["A" + (i + 2)].Value); 
    DictionaryLoad.DIC.Add(key, value);
}
Glen Thomas
  • 10,190
  • 5
  • 33
  • 65
  • I was hoping but I get an error :Use of unassigned local variable, for both key and value meh. Thanks though. – Darw1n34 Sep 14 '15 at 19:58
  • 1
    @Darw1n34 See updated answer... We'll get there eventually! :) I'm just writing this code in my head because I don't have a test application to edit. – Glen Thomas Sep 14 '15 at 19:59
  • ugh so close. it throws this error. Member 'string.Copy(string)' cannot be accessed with an instance reference; qualify it with a type name instead. I appreciate the patience – Darw1n34 Sep 14 '15 at 20:02
  • 1
    @Darw1n34 Ok, got it this time. – Glen Thomas Sep 14 '15 at 20:04
  • Ok that worked, as far as not throwing errors, but it still leaves the Excel application open. – Darw1n34 Sep 14 '15 at 20:10
  • 1
    And if you remove the code that copies the values to the Dictionary it releases the Excel application? – Glen Thomas Sep 14 '15 at 20:14
  • When I wrote the OP, I could comment either the i OR j loop and it would not stick. It only stuck if they were both active code. Now, I just tried it and it sticks around all the time now. ugh back to isolating things. Thanks for your help, unless you have any other suggestions. – Darw1n34 Sep 14 '15 at 20:20
  • A memory profiler might help show you whats holding the reference, but I'm not sure how well that works with COM. I've just noticed that you are adding items from your excel model to a ComboBox too, so that could be another issue. You could try stripping your code back to basics to find where the problem is. I've written Office Addins in the past so I know how painful this can be. – Glen Thomas Sep 14 '15 at 20:25
  • "a reference to string objects linked to the Excel objects" - the .NET strings certainly won't be 'linked' to anything. However, if the `Range` objects were being stored in the `Dictionary` that would be the case. – Govert Sep 14 '15 at 20:40