-1

I would appreciate some opinion on whether the code below is fully thread safe and does not leak the "this" reference? What I'm trying to do is essentially bootstrapping/initialising another service in a background thread using an ExecutorService.

I am just slightly concerned as I've read from somewhere that it is bad practise to start a thread from the constructor as it would leak the "this" reference before the class is fully constructed. 

public class MyService {

private final ExecutorService executorService;
private volatile AnotherService anotherService;
private volatile boolean isReady = false;


public MyService(final ExecutorService executorService) {
    this.executorService = executorService;
    start();
}

private void start() {
    executorService.submit(new Runnable() {
        @Override
        public void run() {
            try {
                anotherService = init();
                isReady = true;
            } catch (Exception e) {
                // do nothing, just retry later
            }
        }
    });
}

private AnotherService init() {
    // some code to initialize
    return AnotherServiceBootstrap.getInstance().bootstrap();
}

// some other methods in class

}

Many thanks in advance!

v3locity
  • 1
  • 2
  • Is your concern that you might create two instances of MyService in two threads simultaneously, and one or both of them might end up with the executorService from the wrong thread? – Paul Hicks Aug 08 '17 at 23:16
  • I think this sort of thing is overly complex and unnecessary. Do you know you'll call that service? Then create or inject the service reference in each instance of the object you create. What benefit is there to all this? – duffymo Aug 08 '17 at 23:18
  • You should use an `AtomicBoolean` instead of `volatile boolean` as explain here https://stackoverflow.com/a/3787435/6138873 – jeanr Aug 08 '17 at 23:21
  • @PaulHicks no, that is not the concern here, the concern is whether there is a case of leaking the "this" reference of the not fully initialised MyService object to another thread – v3locity Aug 08 '17 at 23:22
  • @duffymo Yes, but the other service is a bit awkward to work with, in the sense, it might not bootstrap correctly on first attempt, and the executor in this class is meant to retry the bootstrap until it is successful. Otherwise I totally agree with you that the other service can be injected in for simplicity. – v3locity Aug 08 '17 at 23:24
  • Why is leaking this a risk? It looks like only time it is used is during the constructor. What would the buggy side-effect of a leaked this reference be? – Paul Hicks Aug 08 '17 at 23:25
  • You ought to redesign that other service. Why not make it a true REST service and give each object its own client to call it? You're making it too hard. – duffymo Aug 08 '17 at 23:26
  • To be honest, I'm not sure if there is any risk, but that is what I wanted to find out, whether this code would be problematic. – v3locity Aug 08 '17 at 23:29
  • Unfortunately the other service is external to this project and thus can't be changed. :( So until that bit has been improved, we'll have to have workarounds. – v3locity Aug 08 '17 at 23:30
  • 1
    You should talk to the other service. They should not be designing something that's hard to use. The world is going in the direction of REST microservices. They should be 100% ready to be used. If you have to worry about "bootstrapping", they are doing it wrong. – duffymo Aug 08 '17 at 23:55

1 Answers1

0

I think you do NOT have any risk with your code -

  1. Please refer to this question
  2. Given the explanation in that post, the only risk you had is if someone could access the only final member (executorService) before the constructor exits. However, the only place that variable is getting accessed that way is in the thread that you are creating and submitting to the executor service. And before the submit() method runs, you get a happens-before guarantee which says all changes your constructor made so far happen-before that submitted thread runs. So I believe you are covered.
Ashutosh A
  • 975
  • 7
  • 10