Scopes.SINGLETON + null doesn't work

[GoogleCodeExporter]

Singleton doesn't scope nulls properly. I don't know how legitimate a use case 
this is, but we now 
support null via @Nullable, and it seems reasonable to make Singleton play nice.

We should also validate that the other built-in scopes do nulls okay.

Original issue reported on code.google.com by limpbizkit on 10 Nov 2008 at 8:38

[GoogleCodeExporter]

I investigated what it would take to fix this...

The problem is that for HTTP request and session scopes, there's no way to 
differentiate between an attribute 
that's null and one that's unset. Ie. there's no API equivalent to 
Map.containsKey(). So for null to work, we'd 
have to create a token value that stands-in for null. This is clumsy.

I'm downgrading this to low priority. If it's an actual problem for you or your 
app, please comment below and 
I'll increase the priority. As it stands, I'd like to defer adding the hacks 
that will make this work until there's 
real-world demand.

Original comment by limpbizkit on 27 Nov 2008 at 11:56

• Changed state: New
• Added labels: Priority-Low
• Removed labels: Priority-Medium, Milestone-Release2.0

[GoogleCodeExporter]

I noticed today that a @Singleton @Provides method that returned null was being 
called 
multiple times (only due to some logging). It's at least a little unexpected 
and I can 
imagine it biting someone in the future.

Original comment by chris.no...@gmail.com on 13 Jun 2009 at 10:15

[GoogleCodeExporter]

Im not sure this is something we should support at all. I think it should be an 
error for a @Provides method to 
return null.

Original comment by dha...@gmail.com on 14 Jun 2009 at 1:03

[GoogleCodeExporter]

You mean remove support for @Nullable?

Original comment by chris.no...@gmail.com on 14 Jun 2009 at 2:18

[GoogleCodeExporter]

I'm okay with null. It's legitimate in some situations, such as "who's the 
signed in user?" Some of us don't like 
using DI to manage context, but it's easy to support and it reduces the number 
of lines of code that need to be 
written.

I'll up this to a high priority issue - any volunteers to write the actual 
code? We already have a suppressed test 
for the null singleton situation.

Original comment by limpbizkit on 14 Jun 2009 at 10:34

• Added labels: Priority-High
• Removed labels: Priority-Low

[GoogleCodeExporter]

I'll do it.

Original comment by isaac.q....@gmail.com on 27 Jun 2009 at 7:28

• Changed state: Accepted

[GoogleCodeExporter]

@4 Why would that mean removing support for @Nullable?

Seems counter intuitive to me to write a provides that ends up just returning 
null. 
Even if it does some logic, we'd be better served converting a missing binding 
to null 
as we can do more up front checking if no binding is available and null is 
injected as 
a sentinel.

With a provider returning null, you must relax the @Nullable check to the 
runtime 
failure only.

Original comment by dha...@gmail.com on 27 Jun 2009 at 10:21

[GoogleCodeExporter]

Hey Isaac - if you tackle this, you should also tackle the HTTP scopes as well. 
That's slightly harder due to the 
way we store scoped objects.

Original comment by limpbizkit on 27 Jun 2009 at 10:26

[GoogleCodeExporter]

@dhanji, isn't the whole point of Nullable to allow a null injection, i.e. have 
a Provider 
return null?  An example of how this might be used: In my project we have a 
request 
scoped binding for an authenticated user-type; if it's null then we consider 
the request 
to be unauthenticated.

@limp, yep I'll do that also

Original comment by isaac.q....@gmail.com on 28 Jun 2009 at 2:25

[GoogleCodeExporter]

A first attempt is attached.  It uses two different methods for SINGLETON and 
REQUEST
/ SESSION, but I can swap them or do whatever you all suggest.

I'd like to add tests.  I'm not sure where is best.

It also introduces a test break below, which I can fix by changing the assert, 
but
I'd need some guidance to track down the real cause of the bug.

Thanks all.

    [junit] 2)
testBindNullAsEagerSingleton(com.google.inject.NullableInjectionPointTest)junit.
framework.AssertionFailedError:
Expected "Guice provision errors:
    [junit]
    [junit] 1) null returned by binding at [unknown source]
    [junit]  but com.google.inject.NullableInjectionPointTest$FooField.foo is not
@Nullable
    [junit]   while locating com.google.inject.NullableInjectionPointTest$Foo
    [junit]     for field at
com.google.inject.NullableInjectionPointTest$FooField.foo(NullableInjectionPoint
Test.java:171)
    [junit]   while locating com.google.inject.NullableInjectionPointTest$FooField
    [junit]
    [junit] 1 error" to contain substring "null returned by binding at
com.google.inject.NullableInjectionPointTest"
    [junit]     at com.google.inject.Asserts.assertContains(Asserts.java:64)
    [junit]     at
com.google.inject.NullableInjectionPointTest.testBindNullAsEagerSingleton(Nullab
leInjectionPointTest.java:161)
    [junit]     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    [junit]     at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    [junit]     at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.jav
a:25)

Original comment by isaac.q....@gmail.com on 28 Jun 2009 at 4:04

Attachments:

• null_scope.diff

[GoogleCodeExporter]

I took a look at the patch; very nice!

A few questions:
  - why create a class for the sentinel? It would be simpler and have less overhead to use "public static final 
Object NULL = new Object()"
  - why create a Reference wrapper for singletons? This is inner-loop code so we should avoid unnecessary 
wrapping layers wherever possible (and go with the sentinel approach).

Original comment by limpbizkit on 10 Aug 2009 at 11:58

[GoogleCodeExporter]

1. Just using NULL = new Object() is fine to me as well.
2. Using the Reference wrapper makes the double-checked locking code easier to 
read. 
I can change this to a sentinel though..

Original comment by isaac.q....@gmail.com on 11 Aug 2009 at 2:00

[GoogleCodeExporter]

Ok, made the above changes...i.e. used a sentinel Object for both Scopes.

(We're not worried about Serialization right?  the enum I had before would buy 
us
that for free...)

Original comment by isaac.q....@gmail.com on 11 Aug 2009 at 6:29

Attachments:

• null_scope2.diff

[GoogleCodeExporter]

Two more things:
 - can you write a test case that serializes & deserializes the HTTP session's map and makes sure the right value 
comes out afterwards? You might need to define your NULL sentinel to support 
serialization. Hmm, maybe the 
enum was right afterall. I'm sorry
 - on this field:
         private volatile Object instance;  // non-null, and either of type T or equal to NULL
the doc says "non-null", but the field is null until it's created (which 
happens lazily). Perhaps write a longer doc 
to explain the lifecycle of this field.

Original comment by limpbizkit on 11 Aug 2009 at 6:46

[GoogleCodeExporter]

1. I've modified existing tests in ServletTest for scoped-nulls, and I've added 
a 
test-case for serialization (and reverted back to the enum in ServletScopes).  
Let me 
know if that was what you were thinking of.
2. I've expanded the comment here slightly.

Original comment by isaac.q....@gmail.com on 12 Aug 2009 at 5:42

Attachments:

• null_scope4.diff

[GoogleCodeExporter]

Applied Isaac's patch in r1061. Very nice test Isaac!

Original comment by limpbizkit on 12 Aug 2009 at 7:25

• Changed state: Fixed