Skip to content

Plugins: Fail to load a plugin no more prevent loading of all others #11412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 6, 2023
Merged

Plugins: Fail to load a plugin no more prevent loading of all others #11412

merged 1 commit into from
Dec 6, 2023

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented Nov 30, 2023 •
edited
Loading

When a plugin fail to load and an exception is raised, no plugin was added to the Plugins collection
and so none was added to the menu in the UI.

Will mitigate the case where external plugins not (yet) compatible won't prevent internal plugins to load.

Screenshots

Before

image

After

image

image

Test methodology

Test environment(s)

  • Git Extensions 33.33.33
  • Build b98f960 (Dirty)
  • Git 2.42.0.windows.2
  • Microsoft Windows NT 10.0.22621.0
  • .NET 8.0.0
  • DPI 96dpi (no scaling)
  • Portable: False

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned pmiossec Nov 30, 2023
mstv
mstv approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have not run

RussKie
RussKie reviewed Dec 1, 2023
View reviewed changes
GitUI/Plugin/PluginRegistry.cs Outdated
catch
catch (Exception ex)
{
// no-op
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// no-op

RussKie
RussKie reviewed Dec 1, 2023
View reviewed changes
GitUI/Plugin/PluginRegistry.cs Outdated
DebugHelpers.Fail("Fail to load a plugin. Error:" + ex.Demystify());
return null;
}
}).WhereNotNull().ToArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to allocate, is there?

Suggested change
}).WhereNotNull().ToArray();
}).WhereNotNull();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.WhereNotNull() is misindeted, it should be under .Select.

@RussKie
Copy link
Member

RussKie commented Dec 1, 2023

Nice! I think that'd fix the problem @maraf observed in gitextensions/gitextensions.pluginmanager#73.

What was the exception? Which plugins failed?

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Dec 1, 2023
@pmiossec
Copy link
Member Author

pmiossec commented Dec 1, 2023

Nice! I think that'd fix the problem @maraf observed in gitextensions/gitextensions.pluginmanager#73.

What was the exception? Which plugins failed?

Painful session. It took me a lot of time to find the issue as the exception was swallowed (and we don't have a good logging solution).

With this fix, the exception now loggued is:

---- DEBUG ASSERTION FAILED ----
---- Assert Short Message ----
Fail to load a plugin. Error:Microsoft.VisualStudio.Composition.CompositionFailedException: An exception was thrown while initializing part "GitExtensions.PluginManager.Plugin".
 ---> System.TypeInitializationException: The type initializer for 'GitExtensions.PluginManager.PluginSettings' threw an exception.
 ---> System.TypeLoadException: Could not load type 'GitUIPluginInterfaces.BoolSetting' from assembly 'GitUIPluginInterfaces, Version=33.33.33.0, Culture=neutral, PublicKeyToken=null'.
   at static GitExtensions.PluginManager.PluginSettings()
   --- End of inner exception stack trace ---
   at bool GitExtensions.PluginManager.PluginSettings.get_HasProperties()
   at new GitExtensions.PluginManager.Plugin()
   at object RuntimeMethodHandle.InvokeMethod(object target, Void** arguments, Signature sig, bool isConstructor)
   at object System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(object obj, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---
   at object Microsoft.VisualStudio.Composition.RuntimeExportProviderFactory+RuntimeExportProvider+RuntimePartLifecycleTracker.CreateValue()
   at void Microsoft.VisualStudio.Composition.ExportProvider+PartLifecycleTracker.Create()
   at void Microsoft.VisualStudio.Composition.ExportProvider+PartLifecycleTracker.MoveNext(PartLifecycleState nextState)
   at void Microsoft.VisualStudio.Composition.ExportProvider+PartLifecycleTracker.MoveToState(PartLifecycleState requiredState)
   at object Microsoft.VisualStudio.Composition.ExportProvider+PartLifecycleTracker.GetValueReadyToExpose()
   at ExportInfo Microsoft.VisualStudio.Composition.ExportProvider.CreateExport(ImportDefinition importDefinition, IReadOnlyDictionary<string, object> exportMetadata, TypeRef originalPartTypeRef, TypeRef constructedPartTypeRef, string partSharingBoundary, bool nonSharedInstanceRequired, MemberRef exportingMemberRef)+() => { } [0]
   at IEnumerable<Lazy<object, object>> Microsoft.VisualStudio.Composition.ExportProvider.GetExports(Type type, Type metadataViewType, string contractName)+() => { }
   at void System.Lazy<T>.ViaFactory(LazyThreadSafetyMode mode)
   at void System.Lazy<T>.ExecutionAndPublication(LazyHelper executionAndPublication, bool useDefaultConstructor)
   at T System.Lazy<T>.CreateValue()
   at object Microsoft.VisualStudio.Composition.Export.get_Value()
   at IEnumerable<Lazy<object, object>> Microsoft.VisualStudio.Composition.ExportProvider.GetExports(Type type, Type metadataViewType, string contractName)+() => { }
   at void System.Lazy<T>.ViaFactory(LazyThreadSafetyMode mode)
   at void System.Lazy<T>.ExecutionAndPublication(LazyHelper executionAndPublication, bool useDefaultConstructor)
   at T System.Lazy<T>.CreateValue()
   at void GitUI.PluginRegistry.Initialize()+(Lazy<IGitPlugin> lazy) => { } in C:/Users/pmios/Documents/Perso/Dev/GitHub/gitextensions/GitUI/Plugin/PluginRegistry.cs:line 36
---- Assert Long Message ----

   at GitExtUtils.DebugHelpers.Fail(String message) in C:\gitextensions\GitExtUtils\DebugHelpers.cs:line 26
   at GitUI.PluginRegistry.<>c.<Initialize>b__10_0(Lazy`1 lazy) in C:\gitextensions\GitUI\Plugin\PluginRegistry.cs:line 36
   at System.Linq.Enumerable.SelectArrayIterator`2.MoveNext()
   at System.Linq.LinqExtensions.WhereNotNull[T](IEnumerable`1 source)+MoveNext() in C:\gitextensions\GitExtUtils\Linq\LinqExtensions.cs:line 180
   at GitUI.PluginRegistry.Initialize() in C:\gitextensions\GitUI\Plugin\PluginRegistry.cs:line 46
   at GitUI.CommandsDialogs.FormBrowse.<.ctor>b__50_4() in C:\gitextensions\GitUI\CommandsDialogs\FormBrowse.cs:line 277
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at GitUI.CommandsDialogs.FormBrowse.<.ctor>b__50_4()
   at GitUI.TaskManager.HandleExceptionsAsync(Func`1 asyncAction, Func`2 handleExceptionAsync) in C:\gitextensions\GitExtUtils\GitUI\TaskManager.cs:line 28
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at GitUI.TaskManager.HandleExceptionsAsync(Func`1 asyncAction, Func`2 handleExceptionAsync)
   at GitUI.TaskManager.<>c__DisplayClass13_0.<<FileAndForget>b__0>d.MoveNext() in C:\gitextensions\GitExtUtils\GitUI\TaskManager.cs:line 76
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
   at Microsoft.VisualStudio.Threading.AwaitExtensions.TaskSchedulerAwaiter.<>c.<UnsafeOnCompleted>b__6_0(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

As I said above, I think that's due to #11397 where plugins compatible with previous version located in the user folder are not loading and throw.

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Dec 1, 2023
@pmiossec
Copy link
Member Author

pmiossec commented Dec 1, 2023 •
edited
Loading

Which plugins failed?

I have 2 "plugins" in my user folder: GitExtensions.PluginManager.Plugin & GitExtensions.SolutionRunner.PluginSettings

Yes, it could be the reason why @maraf has no plugin loaded.

FYI, I don't know why and I have not investigated as the fix was solving the issue but sometimes I had some plugins loaded (3 or 4) but never the same ones.

⚠️⚠️⚠️ Thinking about it again, with #11397 breaking change, we have a real problem because the plugin manager won't load and so won't be able to upgrade itself or other plugins to a compatible version 😢 ⚠️⚠️⚠️

@pmiossec pmiossec force-pushed the fix/manage_plugin_loading_failure branch from b801ac7 to 8454b8e Compare December 1, 2023 08:11
@maraf
Copy link
Member

maraf commented Dec 1, 2023

Will the error message be visible in VS when developing a plugin?

@pmiossec
Copy link
Member Author

pmiossec commented Dec 1, 2023

Will the error message be visible in VS when developing a plugin?

It depends how GitExtensions is launched during the plugin is development.
I don't remember but I think it won't be the case 😢
If you launch GE in debug in VS, you will see the error in the output.
But if an instance of GE is launched to load your plugin, you won't.

But I think I will improve this PR with something like that:

image

And when you click, you will have the popup with error (popup to improve work in progress):
image

What do you think?

@maraf
Copy link
Member

maraf commented Dec 1, 2023

Sounds good!

@pmiossec
Copy link
Member Author

pmiossec commented Dec 1, 2023

Updated screenshots in PR description.
FYI, I have tried TaskDialog to have a better user friendly display but have decided to stick to MessageBox due to https://stackoverflow.com/questions/40388291/is-the-way-to-disable-path-shortening-in-windows-taskdialogs-c that may loose some important data. MessageBox is also not perfect but good enough.

Otherwise user will copy the error message somewhere else...
I don't think it requires more time invested.

@mstv
Copy link
Member

mstv commented Dec 1, 2023

and we don't have a good logging solution

You could have a look #10307.

@mstv
Copy link
Member

mstv commented Dec 1, 2023

Will the error message be visible in VS when developing a plugin?

You could use Sysinternals DebugView in order to see Trace output.

gerhardol
gerhardol reviewed Dec 2, 2023
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2 with comments

@pmiossec pmiossec force-pushed the fix/manage_plugin_loading_failure branch from 8e290e8 to a5599f7 Compare December 2, 2023 09:02
RussKie
RussKie reviewed Dec 3, 2023
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool!

GitUI/Plugin/FailToLoadPlugin.cs Outdated
using GitUI.Properties;
using GitUIPluginInterfaces;

namespace GitUI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
namespace GitUI
namespace GitUI;

GitUI/Plugin/FailToLoadPlugin.cs Outdated

namespace GitUI
{
public partial class FailToLoadPlugin : IGitPlugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have to be public?

GitUI/Plugin/FailToLoadPlugin.cs Outdated

namespace GitUI
{
public partial class FailToLoadPlugin : IGitPlugin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of FailedPluginWrapper as a name?

GitUI/Plugin/FailToLoadPlugin.cs Outdated
Comment on lines 10 to 14
private readonly Exception _exception;
[GeneratedRegex("\\\"GitExtensions.([^\"]+)\\\"")]
private static partial Regex PluginNameRegex();
private string _pluginName;
public FailToLoadPlugin(Exception exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] ordering and whitespacing

Suggested change
private readonly Exception _exception;
[GeneratedRegex("\\\"GitExtensions.([^\"]+)\\\"")]
private static partial Regex PluginNameRegex();
private string _pluginName;
public FailToLoadPlugin(Exception exception)
[GeneratedRegex("\\\"GitExtensions.([^\"]+)\\\"")]
private static partial Regex PluginNameRegex();
private readonly Exception _exception;
private string _pluginName;
public FailToLoadPlugin(Exception exception)

GitUI/Plugin/FailToLoadPlugin.cs Outdated
private string _pluginName;
public FailToLoadPlugin(Exception exception)
{
_exception = exception?.Demystify();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need ? here? exception is declared as non-nullable.
Also, the exception must be validated, use ANE.ThrowIfNull().

GitUI/Plugin/FailToLoadPlugin.cs Outdated
if (match.Success)
{
_pluginName = match.Value;
Name = TranslatedStrings.FailedToLoadPlugin + ": " + _pluginName.Substring(0, Math.Min(50, match.Value.Length));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use string interpolation

GitUI/Plugin/FailToLoadPlugin.cs Outdated
_exception = exception?.Demystify();
try
{
Match match = PluginNameRegex().Match(exception.ToString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exception.ToString() is called multiple times.

GitUI/Plugin/PluginRegistry.cs Outdated
}
catch (Exception ex)
{
DebugHelpers.Fail("Fail to load a plugin. Error:" + ex.Demystify());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use string interpolation

GitUI/Plugin/PluginRegistry.cs Outdated
catch (Exception ex)
{
// no-op
DebugHelpers.Fail("Fail to load plugins. Error:" + ex.Demystify());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use string interpolation

GitUI/Plugin/PluginRegistry.cs
return lazy.Value;
}
catch (Exception ex)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get some concrete information, e.g., the plugin assembly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we have no information there provided by the failing code.
I have still changed a little the code here to display the assembly name in the error message if it is extracted successfully from the logs.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Dec 3, 2023
@pmiossec pmiossec force-pushed the fix/manage_plugin_loading_failure branch from a5599f7 to 692f98a Compare December 3, 2023 13:49
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Dec 3, 2023
@pmiossec
Plugins: Fail to load a plugin no more prevent loading of all others
dc42b06 
When a plugin fail to load and an exception is raised,
no plugin was added to the `Plugins` collection
and so none was added to the menu in the UI.

Instead display plugin loading failure in plugin Menu

Will mitigate the case where external plugins not (yet) compatible
won't prevent internal plugins to load.
@pmiossec pmiossec force-pushed the fix/manage_plugin_loading_failure branch from 692f98a to dc42b06 Compare December 3, 2023 13:57
@pmiossec
Copy link
Member Author

pmiossec commented Dec 3, 2023

Done.

mstv
mstv approved these changes Dec 3, 2023
View reviewed changes
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have not run

@RussKie RussKie merged commit a37a45b into gitextensions:master Dec 6, 2023
@ghost ghost added this to the vNext milestone Dec 6, 2023
@RussKie
Copy link
Member

RussKie commented Dec 6, 2023

Thank you!

@pmiossec pmiossec deleted the fix/manage_plugin_loading_failure branch December 6, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie left review comments

@gerhardol gerhardol gerhardol approved these changes

@mstv mstv mstv approved these changes

Assignees

@pmiossec pmiossec

Labels
None yet
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pmiossec @RussKie @maraf @mstv @gerhardol