Skip to content

Conversation

@CADBIMDeveloper
Copy link

Hi @chuongmep

Purpose

This PR fixes the issue of AssemblyLoadContext unloading.

To reproduce the issue on the dev branch create a simple sample project with zero dependencies, except Revit:

using Autodesk.Revit.Attributes;
using Autodesk.Revit.DB;
using Autodesk.Revit.UI;

namespace TestLib;

[Transaction(TransactionMode.Manual)]
public class TestCommand : IExternalCommand
{
    public Result Execute(ExternalCommandData commandData, ref string message, ElementSet elements)
    {
        var loadedLibrariesCount = AppDomain.CurrentDomain
            .GetAssemblies()
            .Count(a => a.GetName().Name == "TestLib");

        TaskDialog.Show("dev", $"Loaded Libraries Count: {loadedLibrariesCount}");

        return Result.Succeeded;
    }
}

On second and run it will show that there are 2 assemblies loaded.

image

Seems .Net is smart enough and does some kind of unloading even if I wrap this in another project like:

using Autodesk.Revit.Attributes;
using Autodesk.Revit.DB;
using Autodesk.Revit.UI;
using TestLib;

namespace TestLibW;

[Transaction(TransactionMode.Manual)]
public class TestCommandW : IExternalCommand
{
    public Result Execute(ExternalCommandData commandData, ref string message, ElementSet elements)
    {
        var testCommand = new TestCommand();

        return testCommand.Execute(commandData, ref message, elements);
    }
}

However, in more sophisticated case it could increase the number each run

Description

If you debug RevitAddinManager you'll see the issue that after the cycle

for (counter = 0; alcWeakRef.IsAlive && (counter < 10); counter++)
{
      ...
}

alcWeakRef.IsAlive is still true because of the following things:

  • there was a hard reference to _activeEc = externalCommand
  • stream and symbolStream where not closed and disposed before unloading the context
  • need deferred

So

  1. I don't set _activeEc any more in that method. It's a bit strange, that field is used in getter/setter of the ActiveEC property, but this property is not used anywhere. Probably, it can be removed, however, I've left it intact, maybe you have some plans on that
  2. Loading assemblies and symbols is extracted into a separate method, streams are created with using var
  3. Context unload moved to finally not to duplicate the code. GC.Collect in a cycle invoked deffered

Additionally:

  • AppDomain.CurrentDomain.AssemblyResolve subscription wrapped into a conditional compilation #if, it's redundant in R25 / R26
  • AssemblyLoadContext has it's name now

Declarations

Check these if you believe they are true

  • This PR fix bug
  • This PR for new feature
  • The codebase is in a better state after this PR
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • Snapshot of UI changes, if any.
  • This PR modifies some build requirements and the readme is updated

Reviewers

IDK, @chuongmep , probably?

I'll be absent since tomorrow evening till Feb 09, feel free to change anything and merge, don't wait for me if it's in general okay for you, but you want to change style or something like that 😊

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@chuongmep
Copy link
Owner

Hi @CADBIMDeveloper , look great . I will review careful for this this request. Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants