Clean up variables package #5411
Conversation
Fix VariablesMap copying not making a deep copy Fix wrong variable being returned during peak variable changes Fix variable name comparator returning wrong value in some cases
TPGamesNL
added
bug
| @Override | ||
| protected boolean save(String name, @Nullable String type, @Nullable byte[] value) { | ||
| synchronized (connectionLock) { | ||
| synchronized (changesWriter) { |
There was a problem hiding this comment.
I'm concerned we might deadlock here if something asks for connection lock in order to wake the writer, but since this is old code it doesn't seem to have been a problem.
There was a problem hiding this comment.
Does Java deadlock when trying to acquire a lock which has already been acquired somewhere up the stack? I thought it had sth for that
There was a problem hiding this comment.
I can't remember if the outer lock will become available for the purposes of notifying when the inner one is waiting, I'll have to check.
| } catch (IOException e) { | ||
| Skript.error("Unable to make a final save of the database '" + databaseName + | ||
| "' (no variables are lost): " + ExceptionUtils.toString(e)); | ||
| // FIXME happens at random - check locks/threads |
There was a problem hiding this comment.
I wonder if this is related to the aforementioned problem of obtaining one lock without the other? I will look into this later.
|
|
||
| // Get the amount of variables loaded by this variables storage object | ||
| int newVariablesLoaded; | ||
| synchronized (TEMP_VARIABLES) { |
There was a problem hiding this comment.
Ideally we would switch to something in main memory rather than synchronizing on all of these to save time.
There was a problem hiding this comment.
What exactly do you mean?
I'm not sure if anything related to the way we synchronize here saves any relevant time, this code is only called once when loading all variables.
There was a problem hiding this comment.
If that's the only time it's used it's probably fine, though if that's the case does it need to be synchronized at all? When would this be accessed by another process?
There was a problem hiding this comment.
As far as I can see, it's accessed by 2 threads: the main Bukkit thread, and the logger thread (that reports the status updates, i.e. Loaded X variables so far...)
…es-cleanup # Conflicts: # src/main/java/ch/njol/skript/variables/Variables.java
* Reformat variables, add comments and Javadocs Fix VariablesMap copying not making a deep copy Fix wrong variable being returned during peak variable changes Fix variable name comparator returning wrong value in some cases * Fix tests, my bad * Some reformatting, added comment on saveTask changes concurrency * Make SerializedVariable (and inner class) fields final --------- Co-authored-by: Pikachu920 <28607612+Pikachu920@users.noreply.github.com> (cherry picked from commit d1839ac)
Description
Cleans up the variables package, a lot of reformatting and refactoring. Adds many Javadocs and regular code comments. Improved readability.
Some functional changes:
Variables.copyLocalVariablesnot perfectly copyingVariables.getVariablewhen a lot of variable changes were being madeThis PR doesn't touch
DatabaseStorage, to avoid more conflicts with #4873.Target Minecraft Versions: any
Requirements: none
Related Issues: #5398