I have a util class, with a private constructor, and all methods being static. The reason being, none of the methods share any objects as the class doesn't store any state. However I'm wondering if these static methods, if invoked in parallel, would cause any requests to block? Does the fact that static variables being stored in shared memory locations affect this? Any help in understanding the underlying execution mechanism would be much appreciated. Thanks in advance.

Below is the class I'm talking about.

public final class PersonUtil {
    private static IPerson person;

    private PersonUtil() {
    }

    public static PersonOutput generatePerson(final PersonInput personInput) {
        if (isParamProvided()) {
            person = new MyPerson();
        } else {
            person = new LegacyPerson();
        }
        return person.create(personInput);
    }

    public static void assignState(final String guid) {
      //call AssignState API for given guid
    }

    public static String getGuidForId(final String id) {
        //call GetGuid API for the given ID
    }

    public static String getPersonNameByGuid(final String guid) {
        // call DescribePerson API
    }
}

CodePudding user response：

Yes, you may have a concurrency problem here when you call generatePerson, because this method is impacting the static field person.

Imagine that you have two threads:

• One calls generatePerson with an argument that causes person = new MyPerson()
• Another calls generatePerson with an argument that causes person = new LegacyPerson()

If both threads are concurrent, then it can happen that:

1. The first thread sets person = new MyPerson()
2. The second thread right after sets person = new LegacyPerson()
3. The first thread calls create() on a person which is LegacyPerson (because thread 2 sets it as such on the previous statement), but it should have called it on an instance of MyPerson instead

To avoid this, you should declare person as volatile (that will ensure that all threads see the same thing when it's time to read the variable) and then you should declare your generatePerson as synchronized, so that only one thread at a time can access it:

private static volatile IPerson person;

public static synchronized PersonOutput generatePerson(final PersonInput personInput) {
    if (isParamProvided()) {
        person = new MyPerson();
    } else {
        person = new LegacyPerson();
    }
    return person.create(personInput);
}

A better solution though would be possible if MyPerson and LegacyPerson don't hold any state. If that was the case, then you could declare two instances as final:

private static final IPerson myPerson = new MyPerson();
private static final IPerson legacyPerson = new LegacyPerson();

And so you change the createPerson method to do this:

public static PersonOutput createPerson(final PersonInput personInput) {
    if (isParamProvided()) {
        return myPerson.create(personInput);
    } else {
        return legacyPerson.create(personInput);
    }
} 

Like that, you never write the static fields concurrently and so your static method becomes agnostic from concurrency issues.

Note: I'm providing the example on createPerson because it's the only method for which you're showing your code. You should apply the same logic on all the API endpoints, without seeing the code we can't know whether there are other concurrency issues or not.

Suggestion: avoid concurrency when you can. It's always a source of flaky issues and should be used only when it's strictly necessary. In the second example that I provided you're avoiding it, try to do the same in the other endpoints.

CodePudding user response：

You won't get a performance problem. If you don't have synchronized blocks or synchronized methods, you won't have any performance problem.

Most probably you will get concurrency problems. If multiple persons are generated concurrently by different threads, only a single person will be assigned to the field and you won't be able to predict witch one.

Even worse, the assigned person will be possibly replaced over the time without any explicit assignments (see the next paragraph).

You will definitely get visibility problems. If one thread assigns the static field person, it won't be visible for another thread for sometime (it could be microseconds or minutes). Define person as volatile to avoid it.

private static volatile IPerson person;

And of course you must ensure, that classes MyPerson and LegacyPerson are thread-safe. All the fields of these two classes must be final or volatile. If it cannot be reached, you will need synchronized blocks including related performance drawbacks.