0

I want to make my Builder pattern as Thread safe But facing issues in that, below is my code:

// Server Side Code 
final class Student { 
  
    // final instance fields 
    private final int id; 
    private final String name; 
    private final String address; 
  
    public Student(Builder builder) 
    { 
        this.id = builder.id; 
        this.name = builder.name; 
        this.address = builder.address; 
    } 
  
    // Static class Builder 
    public static class Builder {
  
        /// instance fields 
        private int id; 
        private String name; 
        private String address; 
  
        public static Builder newInstance() 
        { 
            return new Builder(); 
        } 
  
        private Builder() {} 
  
        // Setter methods 
        public Builder setId(int id) 
        { 
            this.id = id; 
            return this; 
        } 
        public Builder setName(String name) 
        { 
            this.name = name; 
            return this; 
        } 
        public Builder setAddress(String address) 
        { 
            this.address = address; 
            return this; 
        } 
  
        // build method to deal with outer class 
        // to return outer instance 
        public Student build() 
        { 
            return new Student(this); 
        } 
    } 
  
    @Override
    public String toString() 
    { 
        return "id = " + this.id + ", name = " + this.name +  
                               ", address = " + this.address; 
    } 
} 
  


----------

There is another class named StudentReceiver.java in which I am using multithreading:

    class StudentReceiver {

    // volatile student instance to ensure visibility
    // of shared reference to immutable objects
    private volatile Student student;

    public StudentReceiver() throws InterruptedException {

        Thread t1 = new Thread(new Runnable() {
            public void run() {
                student = Student.Builder.newInstance().setId(1).setName("Ram").setAddress("Noida").build();
            }
        });

        Thread t2 = new Thread(new Runnable() {
            public void run() {
                student = Student.Builder.newInstance().setId(2).setName("Shyam").setAddress("Delhi").build();
            }
        });

        t1.start();
        t2.start();
        //t1.join();
        //t2.join();
        
    }

    public Student getStudent() {
        return student;
    }
}


----------

Main class is below from where I am calling these methods:

//Driver class 
public class BuilderDemo { 
 public static void main(String args[]) throws InterruptedException 
 { 
     for(int i=0; i<10;i++)
     {
         StudentReceiver sr = new StudentReceiver();
         
         System.out.println(sr.getStudent());
     }
 } 
}


----------

The output I am getting is like below:

null
null
null
null
null
null
null
null
id = 1, name = Ram, address = Noida
null

Why I am getting null here?? May anyone explain and How to make Builder Pattern thread safe so that it can be used in multithreaaded environment.

Hakan Dilek
  • 2,178
  • 2
  • 23
  • 35
  • 1
    `StudentReceiver` code makes no sense to me. Can you explain why you are doing what you are doing there? – Fildor Jul 02 '20 at 07:40
  • 1
    _"Why I am getting null here?"_ - Because you are accessing the field before it is set. Your Builder is alright. Just a Constructor should not return before it has done its job. – Fildor Jul 02 '20 at 07:42

1 Answers1

1

Your Builder Pattern is not the problem here. The Constructor of StudentReceiver is.

Starting a Thread inside it without joing it there will lead to the object being assigned, possibly and probably before the Thread even started. So the student Field will not be set for quite some time. So much time in fact, that executing the System.out.println(sr.getStudent()); line right after the constructor will (very probably) receive null from getStundent().

The fix would be to either:

  • Not use a separate Thread in the Constructor.
  • Or join the thread inside the Constructor ( which somewhat defeates the Thread's purpose ).

And the Builder class should not be static.

Here is an example of what I'd do:

public interface IBuilder
{
   IBuilder setId( int id );
   // ...
   Student build();
}

final class Student { 
  
    // final instance fields 
    private final int id; 
    // + other fields - left out for brevity
  
    private Student(Builder builder) 
    { 
        this.id = builder.id; 
        // + other fields
    } 

    private static Object builderLock = new Object();
    public static IBuilder getBuilder()
    {
        synchronized(builderLock)
        {
            return new Builder();
        }
    }
  
    // Static class Builder 
    public class Builder implements IBuilder {
  
        // instance fields 
        private int id = -1; 
        // ...  

        private Builder() {} 
  
        // Setter methods 
        public IBuilder setId(int id) { 
            this.id = id; 
            return this; 
        }

        public Student build() {
            return new Student(this);
        }
    }
}

Disclaimer: untested!

Fildor
  • 14,510
  • 4
  • 35
  • 67
  • 1
    Can you reproduce the last two lines of OPs output? 8 times `null`, then it's set, but then `null` again – Lino Jul 02 '20 at 08:27
  • 1
    @Lino-Votedon'tsayThanks No, I cannot. That's mere chance. On any other machine, it may behave differently than on his. – Fildor Jul 02 '20 at 08:28
  • I have added more outputs [here](https://github.com/choudharylakshyaveer/Demo/blob/master/BuilderDemo.java%20outputs) – Lakshyaveer Chaudhary Jul 02 '20 at 09:46
  • If you have additional information, please add it to your question. – Fildor Jul 02 '20 at 09:56
  • https://www.geeksforgeeks.org/builder-pattern-in-java/ I have replicated it from here – Lakshyaveer Chaudhary Jul 02 '20 at 10:03
  • One more thing here is that if I add Thread.sleep(500); after declaring StudentReceiver example then code runs fine... but still a question that is my pattern thread safe as said by @JavaCoder87 in below answer – Lakshyaveer Chaudhary Jul 02 '20 at 10:09
  • The Sleep just makes it possible for the Thread to execute and finish. The problem that a Constructor never should work in that ways still is present. This has nothing to do with your Builder being thread-safe or not. Since a Builder is very unlikely to be used multithreaded, it doesn't have to be thread-safe (in my opinion). I'd just document it not being thread safe and have the client deal with it if they have to. The only thing that could make some sense to be thread-safe is the factory method `newInstance`. – Fildor Jul 02 '20 at 11:26
  • ... in fact, I'd rather have a `public static Builder getBuilder()` method in `Student` than `Builder` have a `newInstance` Method. – Fildor Jul 02 '20 at 11:30
  • @Fildor I am unable to implement in that way that you have suggested in the code... I have uploaded my code on github and here is the [link](https://github.com/choudharylakshyaveer/BuilderPattern). May you please pull request and after correction push back so that I can understand – Lakshyaveer Chaudhary Jul 02 '20 at 13:19