search

Hidendra / Plugin-Metrics

Metrics tools designed for minecraft plugins to gather statistical data
http://mcstats.org
85 stars 79 forks source link

added public method that will cancel the metrics task. #43

Open iain-davis opened 11 years ago

iain-davis commented 11 years ago

This is a possible solution for issue #42. It is the solution I used in my own copy of the plugin.

Background/motivation: When testing my plugin (CommunityBridge) I frequently do some testing using the reload command. This revealed an complaint from the server that CommunityBridge wasn't shutting down its tasks properly. After confirming that I had cancelled my own tasks I eventually realized that the Metrics class was starting its own task that wasn't being cancelled.

Initially, I didn't look closely at the code and assumed that the "disable" method was intended for shutting down the metrics class...eventually I realized that in addition to cancelling the task, it also turned off plugin-metrics entirely for a given server. Oops. Heads up: When I was originally researching the problem I saw (sadly, I don't recall who) other plugin authors who had assumed the same thing...so there may be released plugins out there that are unintentionally/silently turning off metrics server wide.

This is the small bit of code I added that exposes a method for me (and others, hopefully) to cancel a task and I now call metrics.cancelTask() instead of disable().

blablubbabc commented 8 years ago

I know this pull request is pretty old already, but just wanted to comment on it anyways, in case this PR gets considered some day.

While I agree that some sort of 'shutdown' method would be useful to be included, I don't think cancelling the task will actually stop the task, if it is currently already running. From my current understanding of the underlying craftbukkit code, this will only 'mark' the async task for removal and prevent the task from running again. It will not interrupt it in its current execution, or wait for it to finish. Actually, bukkit is already cancelling all tasks for a plugin automatically on disable (see https://github.com/Bukkit/Bukkit/blob/master/src/main/java/org/bukkit/plugin/SimplePluginManager.java#L429), so this additional calling of task.cancel() shouldn't have any additional influence.

So I think a proper solution to the linked issue is to wait for that task's execution to finish (maybe with some timeout) during plugin disable. Something like this: https://gist.github.com/blablubbabc/e884c114484f34cae316c48290b21d8e