0

We are working in an OSGi context and are getting instances of our services via this idiom which includes double-checked locking:

  private volatile ServiceTracker<IMyService, IMyService> serviceTrackerField = null;

  public IMyService getMyService()
  {
    // double-checked locking
    ServiceTracker<IMyService, IMyService> localTracker = this.serviceTrackerField;
    if (localTracker == null)
    {
      synchronized (this)   
      {
        localTracker = this.serviceTrackerField;
        if (localTracker == null)
        {
          localTracker = new ServiceTracker<>(getBundle().getBundleContext(), IMyService.class, null);
          localTracker.open();
          System.out.println("TRACKER = " + localTracker); // X
          this.serviceTrackerField = localTracker;
        }
      }
    }
    return serviceTracker.waitForService(MY_WAIT_TIMEOUT);
  }

I thought I had understood the basic principles (copy to local, use volatile to guarantee order), but still it can happen that the print in line X prints two different instances of the service tracker. (It doesn't seem to be a problem - the service is acquired correctly; but obviously, we are creating more service trackers than we'd need to and that still gives me a bad feeling).

We are currently switching to a different solution, but I still want to understand how this can happen. Can anyone explain what could go wrong? (JRE is Oracle JRE 1.8.0_141 if it matters).

Edit: the Code above is in a bundle activator (actually in Eclipse/Equinox context). So it can be assumed there is only one instance of the class.

Stefan Winkler
  • 3,871
  • 1
  • 18
  • 35
  • 6
    Are you sure that `this` is a singleton? Is it possible that `this` is more than one instance? Try changing the synchronized object to `synchronized(IMyService.class){ ` – John Vint May 07 '20 at 14:15
  • 1
    `private volatile private volatile ServiceTracker this.serviceTrackerField = null;` does not look like a valid Java field declaration. Does this code compile? – Stephen C May 07 '20 at 14:19
  • 4
    @JohnVint when there are multiple instances, just changing the `synchronized` statement wouldn’t be enough. Changing the field to `static` would also be required. But it would be a questionable result, having multiple instances of a class to represent a `static` state. – Holger May 07 '20 at 14:22
  • 1
    Yeah, you're right @Holger. OP would need to also change the field to static. – John Vint May 07 '20 at 14:37
  • Thanks for having a look. There should only be one instance as it is a bundle activator. See edit. – Stefan Winkler May 07 '20 at 20:49
  • 1
    "Should be" doesn't mean "is". The double-checked locking portion is correct so prove that you only have one instance (e.g., include the the instances' default identity hash codes in the log message). – erickson May 07 '20 at 21:33

0 Answers0