1

Folks- I have a helper class, the task of which is to build some messages based on the parameters. The class by itself does not have any private data (other than the instance ofcourse).

public class RequestBuilder {
    private static RequestBuilder instance = new RequestBuilder();

    private RequestBuilder() {}

    public static RequestBuilder getInstance() {
        return instance;
    }

    public SetRequest buildSetRequest(Path prefix,
                                      Path path,
                                      ConfigEntity configEntity,
                                      Any protoAnyData) {
        .....
        .....
        return setRequest;
    }

    public GetRequest buildGetRequest(Path prefix,
                                      Path Path,
                                      RetrieveRequestEntity retrieveRequestEntity,
                                      Encoding encoding) {
        .....
        .....
        return getRequest;
    }
}

I understand singleton classes are not multithreading friendly. In this case, what happens when 2 threads try to execute buildSetRequest() at the same time?

Thanks for your time.

EDIT: Based on my need, and as suggested by @BoristheSpide in the comments below, I'm going to make this class as a utility class with the following changes: 1. Make it final. 2. Make methods static. 3. Remove all singleton references.

public final class RequestBuilder {

    private RequestBuilder() {}

    public static SetRequest buildSetRequest(Path prefix,
                                      Path path,
                                      ConfigEntity configEntity,
                                      Any protoAnyData) {
        .....
        .....
        return setRequest;
    }

    public static GetRequest buildGetRequest(Path prefix,
                                      Path Path,
                                      RetrieveRequestEntity retrieveRequestEntity,
                                      Encoding encoding) {
        .....
        .....
        return getRequest;
    }
}

I'm leaving the original code as is, because it is still valid and gives context to the comments and answers to this question.

SimpleCoder
  • 1,101
  • 2
  • 10
  • 21

1 Answers1

1

In this case, not too much, because your constructor is empty (what others mention as no shared state). Problems start when you have multiple private instance variables that you have to initialize. In that case, you need some protection like the doble-check:

private static volatile RequestBuilder instance;

private RequestBuilder() {}

public static RequestBuilder getInstance() {
    if (instance == null) {
        synchronized (RequestBuilder.class) {
            if (instance == null) {
                instance = new RequestBuilder();
            }
        }
    }
    return instance;
}

The reason is that a thread can be suspended at any time. If the current thread constructing the instance is suspended, and another one comes, there can be half initialized instance variables and the object can end up in a corrupted state.

EDIT: About buildSetRequest()

The code is inside a method, if this method create it's own instances itself or works with thread safe classes, there won't be any problem.

tastydb
  • 542
  • 1
  • 3
  • 13
  • This is just the difference between lazy or eager initialisation. Please read the rules around class initialisers - the code the OP has is perfectly safe from a construction standpoint. – Boris the Spider Feb 28 '19 at 19:29
  • 1
    @BoristheSpider Thanks for the negative vote. Yes, but the OP mixes the fact that's a singleton class with the call to `buildSetRequest()` and I didn't know how to focus the answer. Thanks for your feedback and feel free to expand my explanation. – tastydb Feb 28 '19 at 19:35
  • Sorry, this is not about the focus of your answer. This is about the entire first part of your answer being incorrect. There is no way, given the Java language specification, that a half initialised instance can leak. Your suggestion adds spurious complexity to spurious "facts" about concurrency. – Boris the Spider Feb 28 '19 at 19:40
  • @BoristheSpider Whatever, but what I answered is the correct way to go. Check this and you will see that, effectively, Java memory model allows the publication of partially initialized objects: [https://stackoverflow.com/questions/45857765/partial-constructed-objects-in-the-java-memory-model](https://stackoverflow.com/questions/45857765/partial-constructed-objects-in-the-java-memory-model). Again, you can always answer the OP's question too. – tastydb Feb 28 '19 at 19:53
  • That's a completely different example. You are suggesting that a _class initialiser_ is not protected by the JVM. The constructor call is protected by the class initialiser. I don't know what more to say - this answer is fundamentally incorrect and spreads dangerous misinformation about how Java works. I ask you as a professional to delete it. – Boris the Spider Feb 28 '19 at 21:40
  • @tastydb, in the code you have above, (where you added `volatile`), and I just have it as `private static`, there is already a `new RequestBuilder()`. So, how would instance be `null` in the constructor? – SimpleCoder Feb 28 '19 at 22:15
  • @tastydb. sorry, I meant to ask, how would the instance be `null` in `getInstance()`? – SimpleCoder Feb 28 '19 at 22:53
  • @SimpleCoder it won't. It can't ever be. Please ignore this answer. It confuses a number of advanced concurrency concepts. – Boris the Spider Feb 28 '19 at 22:59
  • @tastydb, could you please fix your answer? They way the code is in the question, `instance` will never be `null` in `getInstance()`. Not many people who hit this question read the comments. They just read the answer. – SimpleCoder Mar 01 '19 at 19:09