CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors #3273

connorivy · 2024-04-09T20:52:20Z

Description & motivation

Changes:

Added DllConlfictManagement project

DllConflictManager

Detects conflicts between assemblies referenced by the connector and currently loaded assemblies in the app domain
- I think we probably need to add this exception handling in more places. I currently only have it for the loading of the connector when Revit starts up

DllConflictUserNotifier

Handles any exceptions that occur, reference them with the conflictManager, and report them to the user. This is an abstract class that is expected to be implemented by the host app to use the host app specific way of notifying a user.

DllConflictManagmentOptions

Holds a list of dll conflicts that the user has elected to ignore
DllConflictManagmentOptionsLoader class will load these options from a json file in the same directory as the connector dll

RevitExternalApplication

Moved all types that are not either connector, System, or Revit types out of the "OnStartup" method.
- This is because if there are conflicting dll types in this method, then an exception will be thrown before the method is even entered
- By putting the other types in different methods, the type loading exception will be thrown on entering that method. Therefore we can alert the user about the exception

To-do before merge:

Questions to be answered (probably by @AlanRynne)

~~Where in appdata should I keep options.json file? Can this be the same for all c# connectors that use this?~~
~~Currently I'm only trying to catch typeLoadException and MissingMethodException. Are there others that will be thrown when dll mismatch occurs?~~
~~I'm only catching the above exceptions in the plugin initialization. I'm thinking we also want to catch these for the operations. Is this needed / is it okay to do?~~
~~How to log while detecting plugin conflicts? Should I prioritize logger dependencies and then move to connector dependencies?~~
~~I'm referencing the System.Text.Json nuget in the new project because it isn't built into .net standard. Should I reference a really new version or a really old version of this?~~

Screenshots:

Validation of changes:

Checklist:

My pull request follows the guidelines in the Contributing guide?
My pull request does not duplicate any other open Pull Requests for the same update/change?
My commits are related to the pull request and do not amend unrelated code or documentation.
My code follows a similar style to existing code.
I have added appropriate tests.
I have updated or added relevant documentation.

References

ConnectorCore/DllConflictManagement/DllConflictManagement.csproj

AlanRynne · 2024-04-12T08:28:27Z

ConnectorCore/DllConflictManagement/DllConflictManagement.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="System.Text.Json" Version="4.6.0" />


Note to @BovineOx and my future self. We may want to consider using Speckle.Newtonsoft considering many more people will use System.Text.Json in varying versions

ha, yeah that's interesting, because System.Text.Json is becoming the more mainstream package, though there are still gaps, but maybe so.

AlanRynne

I've been poking around this new feature in debug and I have a couple considerations:

When a user sees this pop-up, it is unclear what they are expected to do about it.
It is unclear what the consequences are of "not warning about this again"
The checkbox to not warn will apply to each conflict individually, but users can't individually choose which ones to ignore
From Matteo: We should have metrics on when does this pop-up get shown, how many conflicts we find or not, etc...

Further discussions may be needed.

teocomi

I was checking the data in MP:

the distinct_id should either be undefined or yours, but it has some other value
the allDetectedConflicts value should be a list of objects, but it's a list of strings
identifiedConflictingAssemblyName and some other props you're adding are null, not sure if this is correct?

connorivy · 2024-04-22T17:39:50Z

@teocomi,

the distinct id's that you're probably seeing are when I was using a fallback value that didn't come from the accounts (you should know that I pulled this fallback value from the existing Analytics class, so this alternate distinct_id may be being used already). I have changed this and have hard-coded an id of "undefined" for the "lite" version of the analytics.

some system object types (System.Type, System.Reflection.AssemblyName, System.Reflection.Assembly) were throwing when attempting to be serialized so I just made a custom serialization to string. However, I have now implemented a data transfer object which has good serialization behavior, and I am sending that object instead of just a string.

identifiedConflictingAssemblyName can be null if a single conflict was not identified to be the culprit of a thrown exception. Is there a more clear way to handle this?

Here is an example of a couple events. The first is a "DllConflictsDetected" event and the second is a "DllConflictExceptionDialogShown" event

…Handling

BovineOx · 2024-04-23T06:38:51Z

I've been poking around this new feature in debug and I have a couple considerations:

When a user sees this pop-up, it is unclear what they are expected to do about it.

It is unclear what the consequences are of "not warning about this again"

The checkbox to not warn will apply to each conflict individually, but users can't individually choose which ones to ignore

From Matteo: We should have metrics on when does this pop-up get shown, how many conflicts we find or not, etc...

Further discussions may be needed.

For this we could have a blog post and a link to that.

teocomi · 2024-04-23T08:05:54Z

For this we could have a blog post and a link to that.

There's already a docs page about dll conflicts, if you want to link to it we can add some more explanation about the dialog...

AlanRynne

I don't see much blockers. Just one class that should be in it's own file, and a question about the autodesk specific method

ConnectorCore/DllConflictManagement/Analytics/AnalyticsWithoutDependencies.cs

AlanRynne · 2024-04-23T15:55:12Z

ConnectorCore/DllConflictManagement/Analytics/AnalyticsWithoutDependencies.cs

+  [System.Diagnostics.CodeAnalysis.SuppressMessage(
+    "Design",
+    "CA1031:Do not catch general exception types",
+    Justification = "Catching all exceptions to avoid an unobserved exception that could crash the host app"
+  )]


Interesting... could we do something like IsFatal here (though not directly that as it would require referencing something else)

AlanRynne · 2024-04-23T15:56:55Z

ConnectorCore/DllConflictManagement/AssemblyConflictInfoDto.cs

+  private static string GetPathFromAutodeskOrFullPath(string fullPath)
+  {
+    string[] splitOnAutodesk = fullPath.Split(separator, StringSplitOptions.None);
+
+    if (splitOnAutodesk.Length == 2)
+    {
+      return splitOnAutodesk[1];
+    }
+    return fullPath;
+  }


Autodesk specific? 🤔 Ideally this project should work on any connector. Not a deal breaker, just found it weird.

AlanRynne

We're looking good! We should make a WIP to really test this in the wild (maybe ping some DPs and users we know have had conflicts in the past?)

My understanding is that Matteo has already given his blessing to this, though not officially through the PR. So I'm dismissing this to get it through

Connor Ivy added 2 commits April 9, 2024 14:49

add conflict management project

35e75c3

rename namespace

a792905

connorivy requested a review from teocomi as a code owner April 9, 2024 20:52

connorivy changed the title ~~add conflict management project~~ CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors Apr 9, 2024

connorivy requested review from AlanRynne and removed request for teocomi April 9, 2024 20:54

connorivy assigned AlanRynne Apr 9, 2024

don't over catch exception

9a2a77b

connorivy marked this pull request as draft April 10, 2024 12:37

save options to different location

f3d14de

connorivy marked this pull request as ready for review April 10, 2024 18:06

fix: Minor tweaks in csproj for conflictmanager

7e444b9

AlanRynne approved these changes Apr 12, 2024

View reviewed changes

AlanRynne self-requested a review April 12, 2024 08:51

AlanRynne suggested changes Apr 12, 2024

View reviewed changes

Connor Ivy added 2 commits April 18, 2024 18:54

add metrics through events

33d6c50

create undefined user

1358fec

teocomi previously requested changes Apr 22, 2024

View reviewed changes

Connor Ivy added 5 commits April 22, 2024 12:44

add dtos for sending conflict info to mixpanel

0b0c917

Merge remote-tracking branch 'origin/dev' into CNX-8397-DLL-Conflict-…

af77d11

…Handling

fix conflictInfoDto

7dfa78e

Merge remote-tracking branch 'origin/dev' into CNX-8397-DLL-Conflict-…

d1fd72e

…Handling

only track in release mode

888ef8a

AlanRynne reviewed Apr 23, 2024

View reviewed changes

move events to its own file and fix build

2e762b9

AlanRynne self-requested a review April 23, 2024 17:02

AlanRynne approved these changes Apr 23, 2024

View reviewed changes

fix ci errors

0d885c5

AlanRynne merged commit 93dd518 into dev Apr 23, 2024
32 checks passed

AlanRynne deleted the CNX-8397-DLL-Conflict-Handling branch April 23, 2024 20:16

AlanRynne mentioned this pull request Apr 26, 2024

Fix(Revit) : Use speckle newtonsoft instead of System.Text.Json for in dll conflict management project #3317

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors #3273

CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors #3273

connorivy commented Apr 9, 2024 •

edited

Loading

AlanRynne Apr 12, 2024

BovineOx Apr 22, 2024 •

edited

Loading

AlanRynne left a comment

teocomi left a comment

connorivy commented Apr 22, 2024

BovineOx commented Apr 23, 2024

teocomi commented Apr 23, 2024

AlanRynne left a comment

AlanRynne Apr 23, 2024

AlanRynne Apr 23, 2024

AlanRynne left a comment

CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors #3273

CNX-8397-DLL-Conflict-Handling in existing DUI2 connectors #3273

Conversation

connorivy commented Apr 9, 2024 • edited Loading

Description & motivation

Changes:

DllConflictManager

DllConflictUserNotifier

DllConflictManagmentOptions

RevitExternalApplication

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

References

AlanRynne Apr 12, 2024

Choose a reason for hiding this comment

BovineOx Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlanRynne left a comment

Choose a reason for hiding this comment

teocomi left a comment

Choose a reason for hiding this comment

connorivy commented Apr 22, 2024

BovineOx commented Apr 23, 2024

teocomi commented Apr 23, 2024

AlanRynne left a comment

Choose a reason for hiding this comment

AlanRynne Apr 23, 2024

Choose a reason for hiding this comment

AlanRynne Apr 23, 2024

Choose a reason for hiding this comment

AlanRynne left a comment

Choose a reason for hiding this comment

connorivy commented Apr 9, 2024 •

edited

Loading

BovineOx Apr 22, 2024 •

edited

Loading