NOTICE! This is a static HTML version of a legacy ImageJ Trac ticket.

The ImageJ project now uses GitHub Issues for issue tracking.

Please file all new issues there.

Ticket #1980 (closed defect: fixed)

Opened 2013-08-15T10:32:55-05:00

Last modified 2014-05-01T13:16:57-05:00

Consider making services thread-safe

Reported by: curtis Owned by: curtis
Priority: critical Milestone: imagej2-b8-analysis
Component: Core Version: 2.0.0-beta-7
Severity: major Keywords:
Cc: MichaelZinsmaier Blocked By:
Blocking: #1519

Description

Right now, lazily initialized services use a pattern that is not thread-safe. If two threads concurrently call the same service for the first time, it may perform its lazy initialization twice.

We could synchronize methods in various places but we need to be careful not to cause major a performance problem when doing so.

Change History

comment:1 Changed 2013-08-15T10:36:52-05:00 by curtis

  • Cc MichaelZinsmaier added

comment:2 Changed 2013-08-15T10:53:00-05:00 by dscho

It might be best to make methods like this:

public LogService log() {
    if (log == null) synchronized (this) {
        if (log == null) {
            log = getContext().getService(LogService.class);
        }
    }
    return log;
}

That way, we avoid races (by having a check for log == null in the synchronized block) but we also avoid the cost of the synchronized block in the vast majority of the cases, see  http://norvig.com/java-iaq.html#slow

comment:3 Changed 2013-08-15T11:49:57-05:00 by curtis

Thanks dscho. I take back what I said about it being tricky. I even thought about that approach while working on the lazy initialization logic originally, but thought "YAGNI" as well as "it would be ugly", but clearly we *do* need it and it is very straightforward. As for it being ugly, dscho also demonstrated that  it can be written more elegantly.

So we will definitely make similar such changes to the SciJava Common and ImageJ2 core services.

comment:4 Changed 2013-08-15T12:04:45-05:00 by aivar

1) Double checked locking works after Java 1.5 if you make 'log' here volatile.

 https://en.wikipedia.org/wiki/Double-checked_locking

2) Dscho's Norvig link has "Using bitset.get(i) is 60 times slower than bits & 1 << i. This is the cost of a synchronized method call, mostly." However "bits & 1 << i" is an and, a shift, and a few loads, so just a few clock cycles. 60 times that is still negligible.

comment:5 Changed 2013-08-15T12:25:26-05:00 by curtis

Thanks aivar. I take back what I said about taking back what I said about it being tricky. Why is multithreading always so complicated??

Anyway, we can still fix this of course, using volatile and as described on that wiki page. It's just more complex than I would like.

comment:6 Changed 2013-08-15T13:53:52-05:00 by dscho

The inlining of constructors is the problem here. But getContext().getService(...) cannot be inlined, because getService() call performs a real lookup (and hence cannot assign the variable early).

So while it is true that double-checked locking has its problems, only little more research is needed in this case to point out that the pointed out problem is in fact not a problem. Just wanted to point that out.

comment:7 Changed 2013-08-15T17:01:07-05:00 by curtis

  • Milestone changed from imagej-2.0.0 to imagej2-b8-analysis

comment:8 Changed 2013-09-24T15:18:25-05:00 by bdezonia

  • Blocking 1519 added

comment:9 Changed 2014-05-01T13:16:57-05:00 by curtis

  • Status changed from new to closed
  • Resolution set to fixed

We use the double checked locking pattern for all services now, AFAIK.