WIP - Ticker Rewrite #2996
Closed
WIP - Ticker Rewrite #2996
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update code.
This commit changes TickerTask#halt to block and process all remaining tasks.
…n/tasks/TickerTask.java Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Minor changes to the ticker task
|
I think this can now be un-marked as a draft. |
|
|
What... this compiled fine for me yesterday |
…ce/ticker-rewrite
Fix compile error in TickerTask
|
Ok now that this compiles... can it be un-marked as a draft? |
The `processSyncTasks` method had a bug in it which caused an infinite loop. This was running on the main thread (since sync) and thus never terminated.
On shutdown we call `tick()`, this will submit async tasks and process the final sync tasks. In `processSyncTasks` we have a while loop. `while (!plugin.isEnabled() || deadline > System.nanoTime()) {` the plugin is shutting down therefore `isEnabled` will return false meaning this while loop will always trigger. Next we polled the tasks (`syncTasks.poll(MAX_POLL_TIME, TimeUnit.NANOSECONDS);`). When there are none left this will return null. We have a null check and inside that block we haev `if (plugin.isEnabled()) { break; }`. We want to break out of this loop however as mentioned already `isEnabled` will return false and thus causing this infinite loop bug.
|
Kudos, SonarCloud Quality Gate passed! |
|
Things to address: |
|
About thread interruption: I suppose we actually do need to re-interrupt the thread for |
TheBusyBot
added
the
⚡ Merge Conflicts
Re-Interrupt the thread.
|
Is unit testing the only thing missing on this PR or are there other issues to be resolved? |
I wish I knew |
|
closing this as stale. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This Pull Request was originally created by @LinoxGH, see #2290.
We have decided that it is not ready to merged into
masteryet as there is still a lot that needs to be tweaked.That's why I am reopening it under a new branch inside the main repository.
Proposed changes
This makes some changes to the way
TickerTaskschedules its tasks.Checklist
NonnullandNullableannotations to my methods to indicate their behaviour for null values