EDCD / EDCD/EDMarketConnector

Clicking "Disable" multiple times in Plugins tab causes multiple calls to `plugin_stop`

Open
#2,605 2 comments 0 reactions 1 assignee Claimed by @Rixxan View on GitHub
bug Staged
Dominant language
Python
Stars
1.3k
Forks
182
Avg merge
1m
Merged PRs (30d)
1

Description

Version: 6.1.2

From `prefs.py`:
```python
def disable_plugin(self, plugin):
"""Disable an existing plugin and restart."""
logger.debug(f"Calling plugin_stop() for {plugin.name}")
if plugin._get_func('plugin_stop'): # Try to stop cleanly. We're going regardless...
plugin.module.plugin_stop()
# LANG: Text of Notification Popup for EDMC Restart
restart_msg = tr.tl( # LANG: Disabling a Plugin
r"Disabling plugin {PLUGIN}. This will cause a restart. Click OK to continue..."
).format(PLUGIN=plugin.name)
restart_box = tk.messagebox.Message(
title=tr.tl('Restart Required'), # LANG: Title of Notification Popup for EDMC Restart
message=restart_msg,
type=tk.messagebox.OK
)
restart_box.show()
# ...
```

If the plugin doesn't return immediately from `plugin_stop` (e.g. it needs time to save/send data or join threads), the user is able to click the button again during this delay; these events are not immediately processed, but buffered by OS. After the plugin returns from `plugin_stop` and execution continues in `disable_plugin`, the following line introduces the core issue:

```python
restart_box.show()
```

I am not an expert on Tk internals, but as far as I understand, this call opens a modal dialog which pumps the event loop to process pending events in the queue. This means that any "Disable button pressed" events queued prior to `restart_box.show()` will be processed before it, effectively calling `disable_plugin` repeatedly for as many times as the user has pressed the button.

The execution of each call (until the last one) will stall at the `restart_box.show()` line. This may lead to non-critical (on application scale) but unexpected consequences for the plugin, as it likely expects `plugin_stop` to be called only once. Additionally, if the plugin manages to handle this without throwing exceptions, the execution of each `disable_plugin` call will continue and spawn multiple `restart_box` dialogs. Conversely, if the plugin does throw an exception, it will likely only abort that specific execution of `disable_plugin` and not affect EDMC as a whole.

The simplest fix here would be disabling the button immediately upon entering `disable_plugin`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.