DYN-9348 Integrator PackagePath following Integration version #16528
Conversation
After providing the context to GitHub Copilot for updating the PathManager lazy loading singleton and updating the PathManagerParams in the constructor, the next alternatives were provided by Copilot: Set Before First Access (Recommended) - Adding a static Initialize method Pass via IPathResolver Modify the Lazy Initialization Logic Set Static Properties Before First Use Recreate the Singleton (Not Recommended) In this PR is implemented the first one with some minor changes based in GitHub Copilot suggestions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9348
There was a problem hiding this comment.
Pull Request Overview
This PR fixes installing Dynamo packages using the DynamoRevit version-specific path by implementing a singleton initialization pattern for PathManager. The fix allows the PathManager to be initialized with host version information before first access, ensuring the correct package path is used based on the host's major and minor version numbers.
Key changes:
- Added static initialization method for PathManager singleton with version parameters
- Modified DynamoModel constructor to initialize PathManager with host version information
- Replaced immediate lazy initialization with deferred initialization pattern
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/DynamoCore/Models/DynamoModel.cs | Added initialization logic to set PathManager with host version from configuration |
| src/DynamoCore/Configuration/PathManager.cs | Refactored singleton pattern to support initialization with version parameters before first access |
| if (!isInitialized) | ||
| { | ||
| lazy = new Lazy<PathManager>(() => new PathManager(parameters)); | ||
| isInitialized = true; | ||
| } |
There was a problem hiding this comment.
The Initialize method is not thread-safe. Multiple threads could simultaneously check isInitialized as false and both execute the initialization block. Consider using a lock or making this method thread-safe with double-checked locking pattern.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think copilot is bringing this up because one of the main use cases of Lazy<T> is to make initialization thread safe when multiple threads might first initialize an object.
Because the pathmanager was made static - a bad design decision IMO - I imagine that making it lazy tries to be defensive, but the path manager itself is not thread safe AFAIK?
Anyway - my gut feeling is:
- This PR adds minimal complexity to an already hard to reason about piece of Dynamo, so I am okay with that.
- It would be nice to rip this singleton out and rethink it.
- The one thing I will add about alternatives is - should we in DynamoCore really be in control of these paths? It might make sense allow clients to override this by using a new member in the
IPathResolverinterface.
@QilongTang - am I remembering correctly that somehow Civil3d worked around this issue and put packages wherever they wanted? How did they do that?
| if (lazy == null) | ||
| { | ||
| // Fallback to default if not initialized | ||
| lazy = new Lazy<PathManager>(() => new PathManager(new PathManagerParams())); | ||
| isInitialized = true; | ||
| } |
There was a problem hiding this comment.
The Instance property getter is not thread-safe. Multiple threads could simultaneously check lazy == null and both execute the fallback initialization. This could lead to race conditions and inconsistent state.
Copilot uses AI. Check for mistakes.
|
@QilongTang this one was the best solution that Copilot provided for setting the PathManagerParams in the PathManager constructor, I tested the fix in a different branch (before .NET 10 changes) and Revit Preview Release (2025) - using latest changes in DynamoRevit repo. |
| lazy = | ||
| new Lazy<PathManager> | ||
| (() => new PathManager(new PathManagerParams())); | ||
| private static Lazy<PathManager> lazy; |
There was a problem hiding this comment.
Why is this implementation Lazy the docs say:
Use lazy initialization to defer the creation of a large or resource-intensive object, or the execution of a resource-intensive task, particularly when such creation or execution might not occur during the lifetime of the program.
I think it's unlikely any of our clients will not need a path manager.
If it was to make initialization thread safe, well then, this PR breaks that invariant, so whats the point?
|
this PR:
|
|
@RobertGlobant20 can you explain why you need to introduce |
There was a problem hiding this comment.
Hi @RobertGlobant20, after much debugging and considering different options, here are my thoughts and recommendations. Please let me know what you think about them.
The indeterministic nature of PathManager initialization causes it to be initialized with empty PathManagerParams() if PathManager.Instance is accessed before host applications (Revit, Civil3D) have a chance to supply DynamoModel with their desired configurations.
Note that the default empty PathManagerParams() does initialize PathManager with sensible defaults, but these don't necessarily match what the host application has in mind. For example, running Dynamo in Revit gives us the defaults shown below (note that this is without your sample path fix in PR #16578, so we still see ProgramData below):
| Path | Value |
|---|---|
| BackupDirectory | %AppData%\Dynamo\Dynamo Core\backup |
| DefaultPackagesDirectory | %AppData%\Dynamo\Dynamo Core\4.0\packages |
| DefaultUserDefinitions | %AppData%\Dynamo\Dynamo Core\4.0\definitions |
| LogDirectory | %AppData%\Dynamo\Dynamo Core\4.0\Logs |
| PreferenceFilePath | %AppData%\Dynamo\Dynamo Core\4.0\DynamoSettings.xml |
| PythonTemplateFilePath | %AppData%\Dynamo\Dynamo Core\4.0\PythonTemplate.py |
| UserDataDirectory | %AppData%\Dynamo\Dynamo Core\4.0 |
| CommonDataDirectory | %ProgramData%\Dynamo\Dynamo Core\4.0 |
| DefaultTemplatesDirectory | %ProgramData%\Dynamo\Dynamo Core\templates\en-US |
| SamplesDirectory | %ProgramData%\Dynamo\Dynamo Core\samples\en-US |
| DynamoCoreDirectory | %UserProfile%\Documents\dev\Dynamo\bin\AnyCPU\Debug |
Tweaks Needed
I agree with @mjkkirschner 100% that the singleton static PathManager is a bad idea - it needs a design overhaul, and I'd opt to rip this singleton out in a heartbeat. However, we are too close to the release date for that. So here's an alternative I'm proposing to keep the risk to a minimum.
This PR is quite close to the "ideal" way, and is frankly the better approach compared to PR #16460. It just needs a few tweaks:
-
Remove the
bool PathManager.isInitializedvariable - checkingif (lazy == null)is sufficient. -
As Copilot suggested, use a
lockfor thread safety:private static readonly object lockObject = new object(); public static void Initialize(PathManagerParams parameters) { lock (lockObject) { if (lazy != null) { // Or do we want to reset the existing instance? See below for discussions. throw new InvalidOperationException("PathManager has already been initialized."); } lazy = new Lazy<PathManager>(() => new PathManager(parameters)); } } public static PathManager Instance { get { if (lazy == null) { lock (lockObject) { if (lazy == null) { // Fallback to default if not initialized lazy = new Lazy<PathManager>(() => new PathManager(new PathManagerParams())); } } } return lazy.Value; } }
-
In
DynamoModel.CreatePathManager(), we will call your newPathManager.Initialize()method:internal PathManager CreatePathManager(IStartConfiguration config) { var pathManagerParams = new PathManagerParams(); var version = config.HostAnalyticsInfo.HostVersion; if (version != null) { // Use host versions if provided pathManagerParams.MajorFileVersion = version.Major; pathManagerParams.MinorFileVersion = version.Minor; } Dynamo.Core.PathManager.Initialize(pathManagerParams); // Existing code... if (!config.StartInTestMode) { } }
-
After
AssignHostPathAndIPathResolveris called with the host path, the paths will be as shown below, which are quite sensible (again, this is without your sample path fix in PR #16578, so we still seeProgramDatabelow):Path Value BackupDirectory %AppData%\Dynamo\Dynamo Revit\backup DefaultBackupDirectory %AppData%\Dynamo\Dynamo Revit\backup DefaultPackagesDirectory %AppData%\Dynamo\Dynamo Revit\4.0\packages DefaultUserDefinitions %AppData%\Dynamo\Dynamo Revit\4.0\definitions LogDirectory %AppData%\Dynamo\Dynamo Revit\4.0\Logs PreferenceFilePath %AppData%\Dynamo\Dynamo Revit\4.0\DynamoSettings.xml PythonTemplateFilePath %AppData%\Dynamo\Dynamo Revit\4.0\PythonTemplate.py UserDataDirectory %AppData%\Dynamo\Dynamo Revit\4.0 CommonDataDirectory %ProgramData%\Autodesk\RVT 2027\Dynamo\4.0 DefaultTemplatesDirectory %ProgramData%\Autodesk\RVT 2027\Dynamo\templates\en-US SamplesDirectory %ProgramData%\Autodesk\RVT 2027\Dynamo\samples\en-US DynamoCoreDirectory %UserProfile%\Documents\dev\Dynamo\bin\AnyCPU\Debug HostApplicationDirectory %UserProfile%\Documents\dev\DynamoRevit\bin\NET100\Debug\Revit -
Finally,
EnsureDirectoryExistencewill be called to actually create these directories, so there won't be any race conditions - the paths will have the correct values when directories are created.
Final Thoughts
-
We still need to remove the
IPathResolver.CommonDataRootFolderproperty as we move things toAppData. Both Revit and Civil3D are providing this value right now. If we'd like to keep it for compatibility reasons, we may want to mark it as obsolete and ignore its value for now. -
The above
Initializeapproach will likely not make things better for Civil3D. If I'm reading it correctly, they force create aPathManagerbefore they callAcDynamoModel.Start, which instantiates ourDynamoModel, so we lose the chance to properly callInitializewith the right data. We may want to consider makingInitializeoverride and recreate a new instance ofPathManager. Crude, I know, but it would make things work for Civil3D. -
One step closer, great work! Please help close out PR #16460 🙂
Removed the isInitialized flag and added a lockObject, also removed the code from DynamoModel constructor and moved to the CreatePathManager method.
|
@benglin the PathManager Singleton implementation in the last commit was as suggested only I have to initialize inside the if(version != null) otherwise DynamoSandbox was crashing (due that version was null). |
For debugging purposes (when DynamoRevit is reading the info from Dynamo.config) this fix is needed otherwise when DynamoRevit calls GetDynamoPath method if the DynamoRevitVersion vs DynamoVersion doesn't match is returning null. Then with this fix DynamoPath will be always found and the Dynamo button will be shown in Revit.
|
GIF showing the expected behavior after installing a package in Revit. |
| } | ||
| } | ||
|
|
||
| public static PathManager Instance |
There was a problem hiding this comment.
Some comments for public property please
There was a problem hiding this comment.
LGTM with one comment @benglin I think this PR is pending your another look
QilongTang
changed the title
QilongTang
changed the title
|
@RobertGlobant20 Any tests you can add in this PR? |
Adding a test which verifies that the PathManager singleton instance is created with the expected properties (when using empty parameters).
|
There was a problem hiding this comment.
Thanks for accommodating the changes, @RobertGlobant20, good work as always! LGTM! 🚀
4 participants
Purpose
Fix for installing Dynamo packages using the DynamoRevit version path (e.g. C:\Users<user>\AppData\Roaming\Dynamo\Dynamo Revit\27.0\packages)
After providing the context to GitHub Copilot for updating the PathManager lazy loading singleton for changing the PathManagerParams in the constructor (using HostAnalyticsInfo.HostVersion), the next alternatives were provided by Copilot:
In this PR is implemented the first one with some minor changes based in GitHub Copilot suggestions.
Declarations
Check these if you believe they are true
Release Notes
Fix for installing Dynamo packages using the DynamoRevit version path (e.g. C:\Users<user>\AppData\Roaming\Dynamo\Dynamo Revit\27.0\packages)
Reviewers
@QilongTang @zeusongit
FYIs