Fixing eagerSingleton initialization issue

[Aragos, 2009-08-27 17:03:48]

I think I finally found the issue with eager singletons that so many people
reported but no one could properly pinpoint. Here's a test and a fix.

[Aragos, 2009-09-08 22:37:34]

On 2009/08/27 17:03:48, Aragos wrote:
> I think I finally found the issue with eager singletons that so many people
> reported but no one could properly pinpoint. Here's a test and a fix.

Ping. :)

[Brian Stoler, 2009-09-15 18:10:46]

Thanks for jumping on this!

http://codereview.appspot.com/110095/diff/1/8
File src/com/google/gwt/inject/rebind/GinjectorOutputter.java (right):

http://codereview.appspot.com/110095/diff/1/8#newcode188
Line 188: writer.println("private " + typeName + " " + field + " = " + creator +
"();");
Hmm, feels weird for it to not be final.

Actually, doesn't this allow it to be created twice? Once before the field init
is reached (during another eager singleton field init) and once on field init?

Seems like it would be better instead to never set it in the field init.
Instead, leave it empty, and build up a list of eager singletons. Then in the
constructor, just call the getters of all of them (in any order). That will
ensure each is constructed exactly once.

Note that that means that singleton and eager would generate the exact same code
except for the one call in the constructor. (Do we generate a constructor now?)

Could also be an instance init block instead of a constructor if that is easier.

http://codereview.appspot.com/110095/diff/1/3
File test/com/google/gwt/inject/client/eager/EagerSingletonTest.java (right):

http://codereview.appspot.com/110095/diff/1/3#newcode24
Line 24: // Tests http://code.google.com/p/google-gin/issues/detail?id=48.
Can you make it increment a static counter for each constructor call and assert
it's 1 for both? I think it will be 2 right now as per my previous comments.

[Aragos, 2009-09-15 18:25:29]

I'll send in a new patch set with with some additional tests in later today.

http://codereview.appspot.com/110095/diff/1/8
File src/com/google/gwt/inject/rebind/GinjectorOutputter.java (right):

http://codereview.appspot.com/110095/diff/1/8#newcode188
Line 188: writer.println("private " + typeName + " " + field + " = " + creator +
"();");
On 2009/09/15 18:10:46, Brian Stoler wrote:
> Hmm, feels weird for it to not be final.
> 
> Actually, doesn't this allow it to be created twice? Once before the field
init
> is reached (during another eager singleton field init) and once on field init?
> 
> Seems like it would be better instead to never set it in the field init.
> Instead, leave it empty, and build up a list of eager singletons. Then in the
> constructor, just call the getters of all of them (in any order). That will
> ensure each is constructed exactly once.
> 
> Note that that means that singleton and eager would generate the exact same
code
> except for the one call in the constructor. (Do we generate a constructor
now?)
> 
> Could also be an instance init block instead of a constructor if that is
easier.

No, as a matter of fact that would defeat the purpose of this patch. :) As you
can see below, I check whether the field is null. If the constructor returns
"null" then we will call it several times (as often as it is requested).

http://codereview.appspot.com/110095/diff/1/3
File test/com/google/gwt/inject/client/eager/EagerSingletonTest.java (right):

http://codereview.appspot.com/110095/diff/1/3#newcode24
Line 24: // Tests http://code.google.com/p/google-gin/issues/detail?id=48.
On 2009/09/15 18:10:46, Brian Stoler wrote:
> Can you make it increment a static counter for each constructor call and
assert
> it's 1 for both? I think it will be 2 right now as per my previous comments.

I will do that, but the "null" check should guard against this behaviour.

[Brian Stoler, 2009-09-15 20:22:42]

This works but I think the other way may be better. And tests for this would be
great.

http://codereview.appspot.com/110095/diff/2004/2011
File src/com/google/gwt/inject/rebind/GinjectorOutputter.java (right):

http://codereview.appspot.com/110095/diff/2004/2011#newcode188
Line 188: writer.println("private " + typeName + " " + field + " = " + getter +
"();");
This works, but only in a really odd way, which is that the field initializer
conditionally returns the previously set value of the field!

Still seems easier to generate:

private Foo foo;
private Bar bar;

{
  getFoo();
  getBar();
}

This guarantees that foo and bar are initialized regardless of their
cross-dependencies.

[Brian Stoler, 2009-09-15 21:53:52]

LGTM

Thanks for all the quick followup!

http://codereview.appspot.com/110095/diff/2014/3011
File src/com/google/gwt/inject/rebind/GinjectorOutputter.java (right):

http://codereview.appspot.com/110095/diff/2014/3011#newcode182
Line 182: case SINGLETON:
// Intentionally fall through