Concurrency risk of singleton binding. about dagger

[swankjesse]

Double checked locking should be fine. Unfortunately locking is what makes singleton scoping difficult. We have two bad choices:

• Use a shared lock, and reduce concurrency when creating objects
• Use independent locks and risk deadlock

I prefer the shared lock. The fix for this issue is to create that lock (probably just a new Object()) and to change singleton scope to use it. Double checked locking is fine; if checkstyle complains then we should silence checkstyle.

[cgruber]

Do we necessarily have a deadlock if we signal whether a binding is requested for member or constructor injection, separate the flows for members injection and constructor injection, so construction can happen before members injection (of anything un-injected in the current transitive closure of the requested object), and explicitly disallow any cyclic reference among constructors?

I think we can do better than a global lock if we are willing to keep some constraints in place.

[swankjesse]

After thinking this over some, I think we aren't at risk of deadlock because we don't permit circular dependencies. So per-singleton locks suffice.

(That said, we also don't always statically check that no circular dependencies exist, so an application could still deadlock. But such an application would fail if dagger-compiler has validated it.)

[cgruber]

I have a circular dependency analysis change I intend to put in shortly
into the full graph processor. Some point in Feb, this should be in if I
can get this internal crap done fast. (Hoping to finish this stuff this
week, or mid-next at the latest)

[jameswald]

That's interesting, I'm pretty sure I was able to create a circular dependency which did pass the dagger-compiler. It generated a dot file with this graph:

.

That obviously resulted in a stack overflow at runtime. If this isn't supposed to happen I can reproduce it tonight and try to fix it. The gist of it is that I injected the service into the application via an @Inject constructor, and in turn, the service injected the application via a @Module provider.

[swankjesse]

@cgruber what's your change do? ProblemDetector already tries to do this.

[cgruber]

Hmm. I just walked the dependency graph passing in a stack of the
current path from the root, blowing up if the current node appeared in
the stack. Really simple.

[swankjesse]

Yeah, that's what ProblemDetector does.

[cgruber]

Whoops. Then why aren't we failing at compile-time now… are we just
not applying it in full graph processor?

[swankjesse]

Nope, we aren't doing that yet. We should! It would just require a call to ObjectGraph.validate().

[cgruber]

<facepalm/> 

[cgruber]

The cycle-detection issue is fixed in #163. The singleton concurrency protection still needs to happen.

[JakeWharton]

Whoops. Apparently this isn't fully done just yet.