Skip to content
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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

iain-davis
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants