Ticket #1980 (closed defect: fixed)

Opened 6 years ago

Last modified 5 years ago

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 6 years ago by curtis

  • Cc MichaelZinsmaier added

comment:2 Changed 6 years ago 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 6 years ago 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 6 years ago 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 6 years ago 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 6 years ago 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 6 years ago by curtis

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

comment:8 Changed 5 years ago by bdezonia

  • Blocking 1519 added

comment:9 Changed 5 years ago by curtis

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

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

Note: See TracTickets for help on using tickets.