Modify

Opened 3 years ago

Closed 3 years ago

#18277 closed enhancement (fixed)

[RFC PATCH] Allow plugins to implement Destroyable if they want to allow restartless updates/removals

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 19.11
Component: Core Version:
Keywords: plugin, update Cc:

Description

Use case: there is a plugin update, but the user doesn't want to restart JOSM (maybe they have a lot of unsaved work that they cannot save right now, e.g. they have an incomplete rework of a complicated intersection).
Use case: A plugin crashes, but it implements Destroyable and there is an update for that plugin.

I don't know if I went about the implementation of this the best way, but it works for my plugin (with some modifications on the plugin side).

Attachments (6)

18277.patch (4.2 KB) - added by taylor.smock 3 years ago.
Initial implementation (no tests)
18277.1.patch (4.2 KB) - added by taylor.smock 3 years ago.
Move calls for plugin destroy functions out of PluginPreference.java
18277.2.patch (4.4 KB) - added by taylor.smock 3 years ago.
Catch exceptions thrown by plugins with a destroy method and indicate that a restart should occur
18277.3.patch (7.0 KB) - added by taylor.smock 3 years ago.
Change returnBoolean to restartNeeded, log error when a plugin can't destroy itself, and add initial test (currently fails -- there is a plugin that implements destroyable already, but I don't know if that is the cause of the failure).
18277.4.patch (7.3 KB) - added by taylor.smock 3 years ago.
Fix test (still fails due to kenzi3d issues, but it isn't the destroyable part of the test). Also remove destroyed plugins from the pluginList in PluginHandler.
18277.5.patch (10.6 KB) - added by taylor.smock 3 years ago.
Add methods to ignore known failing plugins (see https://josm.openstreetmap.de/wiki/IntegrationTestIgnores?version=228 -- kenzi3d was consistently failing on my local machine)

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by taylor.smock

Attachment: 18277.patch added

Initial implementation (no tests)

comment:1 Changed 3 years ago by taylor.smock

Keywords: plugin update added

comment:2 Changed 3 years ago by Don-vip

Looks nice! Do you plan to write some tests?

comment:3 Changed 3 years ago by taylor.smock

If I didn't have to change the implementation, yes. I haven't looked at the tests for the PluginHandler yet, so I don't have a timeframe on that.

I'll probably modify the patch so that I'm not calling the destroyables inside the PluginPreference#ok class (e.g., PluginHandler.removePlugins(PluginList)).

comment:4 Changed 3 years ago by Don-vip

Milestone: 19.11

OK. Just one note, the collect is probably unneeded, you can call the destroy method on the stream directly.

Changed 3 years ago by taylor.smock

Attachment: 18277.1.patch added

Move calls for plugin destroy functions out of PluginPreference.java

Changed 3 years ago by taylor.smock

Attachment: 18277.2.patch added

Catch exceptions thrown by plugins with a destroy method and indicate that a restart should occur

comment:5 in reply to:  4 Changed 3 years ago by taylor.smock

Replying to Don-vip:

OK. Just one note, the collect is probably unneeded, you can call the destroy method on the stream directly.

I was thinking of using the list to check if there were any plugins that did not have a destroy method, and return true if that were the case.

I did that in the last two patches, although the last patch is a bit more hardened against a plugin throwing an exception during destroy (which shouldn't happen, but bugs).

comment:6 Changed 3 years ago by Don-vip

Please log the exception. We should know when a plugin fails to destroy itself. Also we should get a better variable name for returnBoolean.

Changed 3 years ago by taylor.smock

Attachment: 18277.3.patch added

Change returnBoolean to restartNeeded, log error when a plugin can't destroy itself, and add initial test (currently fails -- there is a plugin that implements destroyable already, but I don't know if that is the cause of the failure).

Changed 3 years ago by taylor.smock

Attachment: 18277.4.patch added

Fix test (still fails due to kenzi3d issues, but it isn't the destroyable part of the test). Also remove destroyed plugins from the pluginList in PluginHandler.

comment:7 Changed 3 years ago by taylor.smock

The only two plugins that currently implement Destroyable are MapWithAI and Tagging Preset Tester, although the latter "implements" it through extending JosmAction (so not intentionally).

comment:8 Changed 3 years ago by Don-vip

If you have trouble with some specific plugins, you can update PluginHandlerTestIT so that it reads ignored lines from IntegrationTestIgnores.

Changed 3 years ago by taylor.smock

Attachment: 18277.5.patch added

Add methods to ignore known failing plugins (see https://josm.openstreetmap.de/wiki/IntegrationTestIgnores?version=228 -- kenzi3d was consistently failing on my local machine)

comment:9 Changed 3 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 15508/josm:

fix #18277 - Allow plugins to implement Destroyable if they want to allow restartless updates/removals (patch by taylor.smock)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.