2

I'm trying to make an application ported to Java threadsafe and as such am looking to minimise the amount of static variables required.

I will show an example of an issue I want clarity on. I have a class Eparams as follows:

public class Eparams {
    private double a;

    public double getA() {
        return a;
    }
    public void setA(double a) {
        this.a = a;
    }
}

I set the value in another class, Epi.

public class Epi {

    static Eparams eparams = new Eparams();

    public void epi() {
        eparams.setA(3.44);
    }

    public Eparams getEparams() {
        return eparams;
    }
}

I want to access the value of a of type Eparams in another class, EpiParts:

public class EpiParts {

    public void test() {
        Epi epi = new Epi();

        Eparams eparams = epi.getEparams();

        double val= eparams.getA();
        System.out.print(val);
    }
}

I need Eparams values to be non-static and there are multiple threads accessing this class. Is the way I've done it the best way to achieve this?

If I declare in Epi a new instance of Eparams to be static, what are the implications of threads accessing this instance? Making this instance static was the only way I got it to work.

Is this the wrong way to go about this? Is there an easier way to retrieve values across different classes in a threadsafe manner (apart from function arguments and return values)?

Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
arsenal88
  • 1,040
  • 2
  • 15
  • 32
  • you probably want to synchronize the access to `a` – Lino Jul 26 '18 at 12:13
  • you might want to take a look at [synchronization](https://docs.oracle.com/javase/tutorial/essential/concurrency/sync.html). You could provide synchronized methods for accessing (getters) or altering (setters) instance variables – ichantz Jul 26 '18 at 12:24

4 Answers4

2

If I declare in Epi a new instance of Eparams to be static, what are the implications of threads accessing this instance? Making this instance static was the only way I got it to work.

static doesn't make a member thread-safe in any way, just simplifies access to it via the class name.

Is this the wrong way to go about this?

Yes, now the access to a isn't secure at all.

The options I would suggest here:

  1. Using synchronized accessors.

  2. Using an AtomicLong variable with the Double.longBitsToDouble(long) and Double.doubleToLongBits(double) conversions for the getter and setter respectively.

Is there an easier way ... ?

Consider the first method, it's simple and strict.

Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
1

The structure you have is not really thread-unsafe because it won't actually crash or do very strange thing under multi-thread/heavy load.

The only issue you have is that multiple threads accessing a may see stale values. The simplest answer to that is to make a volatile.

public class Eparams {
    // Volatile to ensure `happens before`.
    private volatile double a;

    public double getA() {
        return a;
    }
    public void setA(double a) {
        this.a = a;
    }
}

public class Epi {
    // No escape will happen here.
    Eparams eparams = new Eparams();

    // Note that this is never called - is that deliberate or should this be a constructor?
    public void epi() {
        eparams.setA(3.44);
    }

    public Eparams getEparams() {
        return eparams;
    }
}

public class EpiParts {

    public void test() {
        Epi epi = new Epi();

        Eparams eparams = epi.getEparams();

        double val= eparams.getA();
        System.out.print(val);
    }
}
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
0

The part with the static Eparams eparams should not play a big role. The only way the threads could interact with each other, is while accessing the members of Eparams itself, e.g. setting the value of a.

This altough can be easily solved by using the synchronized keyword on the getter and setter:

public synchronized double getA() {
    return a;
}

public synchronized void setA(double a) {
    this.a = a;
}

More on synchronized can be found in this other question.

Lino
  • 19,604
  • 6
  • 47
  • 65
  • Yes I considered adding synchronized keyword but this would not be fitting with the application I am building. Multiple threads need access and making getters/setters synchronized would add an too much time lag for the applications purpose. – arsenal88 Jul 26 '18 at 12:55
  • @arsenal88 that is the only way tough. If you want consistent data you *have to* synchronize somewhere, else you'll get collisions and inconsistencies very fast – Lino Jul 26 '18 at 12:56
  • @Lino Synchronized is not the only way of thread-safety, final values and copies are some other examples. @arsenal88 Note that `synchronized` does not automatically make your code thread-safe, it just restricts other threads from entering the synchronized block at the same time. Eg. if something reads `Eparams.a` then immediately another thread sets `Eparams.a` to a new value, is the value that was first read still valid? – xtratic Jul 26 '18 at 13:09
  • @xtratic of course but in the example from OP `final` values don't make sense that's why `synchronized` access should be used. But I agree that `immutability` is probably most times preferred – Lino Jul 26 '18 at 13:13
  • I only see `Eparams` being set to a literal value `eparams.setA(3.44);` so it's possible that `final` could work for OP. As for synchronizing, what benefit does it actually provide in this case? Synchronizing on each method separately means one could be reading at the same time another is writing. Why even synchronize them if that's how you're going to handle it? – xtratic Jul 26 '18 at 13:23
0
public class Epi {
    static Eparams eparams = new Eparams();

Eparams being static means that all Epi instances are referencing the same Epi. Thus if Epi instances are running in different threads and updating Eparams.setA() then you'll have a problem.

My recommendation when multi-threading is to use final variables wherever possible. If Eparams was immutable then you'd be thread safe. Does the value in Eparams actually need to change from its initial value? If not, then I'd do something like this instead.

public class Eparams {
    final double a;
    public Eparams(double a){
        this.a = a;
    }
    public double getA(){
        return a;
    }
}

When you do need something mutable and shared between threads then make sure it is synchronized or atomically updated and document that the value may be modified by multiple threads and may change at any time.

Also be sure about what you are exposing with your interface: With public Eparams getEparams(), do you really want to give EpiParts the reference to the Eparams that the Epi instances are sharing? Doing this means that EpiParts can now call Eparams.setA() and get in on modifying the value too.

I would go with something like the following. Hopefully its close to what you're going for:

// immutable is thread-safe
// make all fields final to make `Eparams` immutable
public class Eparams {
    private final double a;
    public Eparams(double a){ this.a = a; }
    public double getA(){
        return a;
    }
}

public class Epi {
    // shared between all instances of `Epi` but is immutable
    // also final so that nothing swaps out a new `Eparams` unexpectedly
    // which would be thread-unsafe
    private static final Eparams eparams = new Eparams(3.44);

    // it is safe to give strangers a reference to your immutable field
    /** Eparams is a constant */
    public static getEparams() {
        return eparams;
    }

    // if `Eparams` were mutable then be careful about giving 
    // it to strangers and instead do something like this
    /** Eparams is mutable and shared between `Epi` instances
        it's values may change at any moment, be careful.. */
    public static getEparamsA() {
        return eparams.getA();
    }
}
xtratic
  • 4,600
  • 2
  • 14
  • 32