Safely setting variable from a diferent thread

So I have a background thread/async task that I use to parse some files, when it finishes it creates an instance of a class, sets the data to that class and replace the pointer I have in my main class with the newly made class from the thread... an example of what I mean:

Code:

// Main class
static Storage storage;

// onEnable and a command can call at any time:
new Parse();


// Parse class
public Parse()
{
    new Thread(this).start();
}
public void run()
{
    Storage storage = new Storage();
    // read stuff from files and store them in storage...

    Main.storage = storage; // << THIS, updating the storage from main class...
}

I'm not sure that's thread safe to do, my question is how can I make it thread-safe ?
That storage pointer will be used by other methods while the background task works but I currently don't have the code that does that so I can't really test it.

 

Digi
If your going to be changing your pointer make your storage variable volatile so threads don't access a cache copy of your storage variable which could be outdated.

Code:java

 
static volatile Storage storage;

As for making it thread safe make sure that the variables inside your Storage class which are going to be accessed by multiple threads are also declared as volatile so that all threads accessing the class will see the updated value.

 

I've got additional question. Let's say main thread is currently executing on of Storage methods, and storage thread changes pointer in the middle of the main thread execution. Will main thread finish this method normally using old object ?

 

Well I will only call that thread when I want to change to a new object, the data inside it will only be accessed by the main thread.

Does volatile affect performance of main thread in any way ? Would using a sychronized block (and how?) help that ?

Also, the update is not time critical, the main thread can have the older copy for a few more seconds, the performance is important though.

I am also curious about raGan.'s question too :}

 

I think the best way to do this would be only chancing the variables on the main class using the main thread, and if an other thread than the main need to do it, use a sync task to do this

 

Digi
Volatile does slow performance slightly, however I'm not sure how long threads keep their cache memory, it could be indefinite so I wouldn't risk not using volatile. If the update is not time critical, and having outdated values is not a problem for your plugin I would consider using a 'lazy set'. Synchronized methods work in a similar way to volatile. However a volatile variable will never block as it is only performing read/write operations whereas inside a synchronized block the thread can block if you set it up to do so.

Out of interest what is your plugin doing with this data, and what is it?

raGan.
I'm not exactly sure but my guess would be that if a method is already running, it would finish that execution first and then update the pointer, provided the class is properly synchronized.

 

Then it needs to be written in such manner. But what does it mean ? Would I need semaphores or something similar ?

 

raGan.
No, just the methods that will be accessed by different threads should be declared as synchronized so that only thread can run inside them at a time. This way you avoid problems where you have two threads in a method at the same time doing different things.

 

Adamki11s
I was thinking about one thread executing a method while pointer is being updated by other thread. Synchronizing method would probably not help, since updating pointer has nothing to do with that method and only one thread is using it.

 

Adamki11s
They're crafting recipes read from any number of files which the plugin must read when starting up and when administrator uses a rescan command. The thread only performs once on request to build the new storage class then dies, it's not constantly updating.
The variables from the storage class are lists/hashmaps that store the recipes, they can be accessed alot of times per second or stay idle for long times (if nobody is crafting), so performance is important and they're not really critical to be up2date.

So you say by using a synchronized block I practically make all crafting tables spit errors because the storage pointer is locked ?


ferrybig
The task won't do anything because I'm trying to not block the main thread with expensive computing

 

Digi
Would it not be easier to hold a static reference to the class and then just update all of the values whenever the rescan command is performed. If all you are then doing is simple reads from your hashmap that shouldn't cause any problems.

 

I am holding a static reference and I am using it, linkage is not the problem, the problem is the multi-threading which I am not familiar with. I want to make the file processing in another thread because if the plugin processes to many files it starts to lag or even pause the server (which someone actually did by adding thousands of recipes).

The lists/hashmaps from the storage object are read from the same thread and I only asked performance-wise if they're affected.

I'll just use volatile on the pointer for now and when I can actually test it I will post again if there are any problems

 

Digi
Ok .

Now that I have a better understanding of your situation I'd recommend you do the following. Since setting a variable as volatile causes all read and writes to go straight to main memory (instead of being cached) this may not be the most efficient solution for your case.

What may be better is to just have two hashmaps in your storage class. Then when you update the recipes make it load all the recipes into the temporary hashmap, then you could hold a lock on the getter methods while the temporary hashmap is copied over to the primary one, this should be milliseconds so shouldn't be a problem. In case this isn't clear here is some pseudo code .

Code:java

 
class Storage {
 
volatile boolean updatingMaps = false, copyingMap = false;
HashMap primary, temp;
 
public void load(boolean init) {
if (init) {
// first run so load into primary
} else {
updatingMaps = true;
 
copyingMap = true;
//copy hashmap into temporary
temp = new HashMap(primary);
copyingMap = false;
 
primary.clear();
//load data into primary hashmap
updatingMaps = false;
//let the recipies be pulled from the primary map
temp.clear();
}
}
 
public Recipe getRecipe(String key) {
if (updatingMaps) {
while (copyingMap) {
// block while the hashmap is being copied
}
//get the temporary key (if the player happens to ask for a recipe while we are reading the config
return temp.get(key);
} else {
//return as normal
return primary.get(key);
}
}
 
}
 

 

If a boolean volatile is fast then I could just use that to block all communication with the pointer and update it directly instead of making clones, no ?

 

Digi
A boolean volatile would have considerably less impact on performance yes, almost unnoticable.

You could do that and just have the recipe return null if it is being updated. However depending on if the load time could be greater than 1 minute you may want to implement the solution above so the player can always access a recipe. I just think its nicer to always have a response for the Player instead of 'Sorry the config is being updated please try again shortly' .