Should Provider.get() throw?

[GoogleCodeExporter]

currently Provider.get() does not throw any (checked) Exception, should it?

Original issue reported on code.google.com by Larry.Ca...@gmail.com on 16 Jun 2009 at 11:21

[GoogleCodeExporter]

Naw, unchecked exceptions are more appropriate here, I think...

Original comment by gavin.k...@gmail.com on 16 Jun 2009 at 11:22

[GoogleCodeExporter]

Guice has an extension that allows provider-like-things to checked exceptions. 
It's most interesting feature is 
that the failures are scoped.
  http://code.google.com/p/google-guice/wiki/ThrowingProviders

A more interesting question along the same lines: should Provider.get() define 
which RuntimeException it 
throws? Guice's Provider.get() always throws a ProvisionException that wraps 
any underlying exception. The 
motivation is to make it easy to change providers independently of their 
corresponding injection points. 

Consider the alternative. In this code, the thrown exception is not wrapped:
  private final Provider<Connection> connectionProvider;

  public String doWork(String value) {
    try {
       Connection c = connectionProvider.get();
       return c.doWork(value);
     } catch (HttpConnectionFailedException e) {
       return someDefaultValue();
     }
  }
As a consequence, the injection point is now tightly-coupled to the particular 
implementation of Provider in 
use. In this case, an HTTP connection provider.

Original comment by limpbizkit on 17 Jun 2009 at 12:21

[GoogleCodeExporter]

I think unchecked exceptions should pass straight through. Checked exceptions 
would
need to be wrapped, and I suppose that the spec could define the unchecked 
exception
that should be used to wrap them but I'm not sure really how important that 
is...

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 12:28

[GoogleCodeExporter]

I'm against any checked exceptions in the get() method signature, but perhaps 
for a known runtime derivative 
for a) wrapping the checked that the provider may encounter, and b) passing on 
container errors, which in this 
case would be "provision failed" for infrastructural reasons (like the 
associated ThreadLocal has something 
essential missing). 

Original comment by PaulHamm...@gmail.com on 17 Jun 2009 at 11:47

[GoogleCodeExporter]

Throwing Exception, Throwable, or some other checked wrapper type does more 
harm than good. Compare 
Callable and Runnable, for example.

It might be cool if we could declare Provider to throw specific checked 
exception types, but the language just 
doesn't support this well enough yet, and it won't in Java 7 either so far as I 
know.

We'd need parameterized exception types so Provider could be declared as:

  public interface Provider<T, throws X> {
    T get() throws X;
  }

Then, you could declare Provider<Foo>, Provider<Foo, throws Bar>, and 
Provider<Foo, throws Bar | Tee>.

If you try to using normal generic type params for exceptions, you either have 
to define your own 
subinterface, or you always have to spell out the exception everywhere, and you 
can only specify one 
exception type. Instead of Provider<Foo>, you might always have to say 
Provider<Foo, RuntimeException>. 
Given that we don't use checked exceptions in the vast majority of cases, this 
isn't worth it.

ThrowingProvider is better than adding a checked exception type parameter to 
Provider itself, but I don't 
believe it carries its own weight. Doesn't it only save one method decl? 
Instead of:

  public interface FeedProvider<T> {
    public T get() throws FeedUnavailableException;
  }

You have:

  public interface FeedProvider<T> extends ThrowingProvider<T, FeedUnavailableException> {}

The first version is clearer, and you can specify more than one exception type. 
Using an exception parameter 
type would confuse many developers since the pattern is used so rarely. 
ThrowingProvider requires 
specialized configuration and increases the surface area of the configuration 
API.

I don't think we should specify anything about runtime exceptions and how 
they're wrapped because this is 
largely injector-dependent. If I were going to specify it, I might say that 
exceptions from "user code" are 
wrapped in a ProvisionException so you can differentiate them from framework 
exceptions, but what 
qualifies as "user code"? Is an extension user code or framework code? From a 
user's perspective, it would 
be framework code, but from the framework's perspective, it's user code (at 
least in Guice).

Rather than constraining injector impls by specifying the runtime exception 
behavior and encouraging users 
to catch runtime exceptions (which can be wrapped arbitrarily deeply), I think 
we should encourage users to 
define their own interfaces with checked exceptions instead of using 
Provider<T>. Just define:

  public interface FeedProvider<T> {
    public T get() throws FeedUnavailableException;
  }

Create a binding to an implementation of it, and you're done.

Original comment by crazybob...@gmail.com on 19 Jun 2009 at 7:47

[GoogleCodeExporter]

How does one scope the return type of a FeedProvider<T> ? That's the utility of 
ThrowingProviders.

Original comment by limpbizkit on 19 Jun 2009 at 8:16

[GoogleCodeExporter]

Good point. I forgot about scoping the result. Unfortunately, that doesn't 
outweigh
the drawbacks. On the bright side, you could easily scope the result of any
no-argument method using a method interceptor. For example, you could support
something like this:

  public class MyFeedProvider implements FeedProvider<MyFeed> {
    @SessionScoped // Caches MyFeed in the HTTP session.
    public MyFeed get() throws FeedUnavailableException {
      ...
    }
  }

Original comment by crazybob...@gmail.com on 22 Jun 2009 at 7:32

[GoogleCodeExporter]

So, in the cirumstance where a Provider implementation needs to signal an error 
to 
it's caller (presumably an Injection framework), how shall we specify a 
(portable) 
mechanism for developers to achieve this?

Original comment by Larry.Ca...@gmail.com on 22 Jun 2009 at 7:40

[GoogleCodeExporter]

FWIW, the Provider in this API is only intended to be injected as a dependency, 
not
to be implemented by users in order to provide instances to the framework. 
Using one
interface for both tasks in Guice has actually resulted in some confusion. 

If you want to signal errors that the user should handle, I would define and
implement a custom interface that throws a checked exception. I'll add this 
advice
along with an example to Provider's documentation.

Original comment by crazybob...@gmail.com on 22 Jun 2009 at 8:18

[GoogleCodeExporter]

color me similarly confused ... I understood that they would be injected, I 
guess I 
did not understand "Where Providers come from" ... I guess I will go look into 
this 
further to get educated ...

Thanks...

Original comment by Larry.Ca...@gmail.com on 22 Jun 2009 at 8:39

[GoogleCodeExporter]

Should the spec wrap unchecked exceptions that occurred during object 
provision? Such as exceptions thrown 
by constructors? In comment #2 I described the utility of doing so...

Original comment by limpbizkit on 22 Jun 2009 at 9:11

[GoogleCodeExporter]

I guess I am confused, I guess I am missing something here, why would we not 
also
include the ProvisionException as Guice does to enable wrapping of checked 
implementation exceptions to be thrown from the Provider impl?

Original comment by Larry.Ca...@gmail.com on 22 Jun 2009 at 9:24

[GoogleCodeExporter]

After a discussion with Bob, I've decided that the drawbacks of wrapping make 
it less appealing. Most notably, it 
makes it possible to accidentally swallow programming errors. Ie. catching 
ProvisionException isn't much better 
than catching RuntimeException.

Original comment by limpbizkit on 23 Jun 2009 at 9:29

[GoogleCodeExporter]

OK, so the outcome is?

Are we saying that the specification shall remain 'silent' on exception 
handling in
Provider.get() or what?

It would have been good if you could have had that discussion where others 
could 
observe.

Original comment by Larry.Ca...@gmail.com on 23 Jun 2009 at 10:27

[GoogleCodeExporter]

Larry, I just pointed out to Jesse via IM that catching "ProvisionException" is 
no
better than catching RuntimeException (assuming they wrap exceptions that came 
from
user code like an injected method or custom factory).

I propose that we only need this on Provider.get():

     * @throws RuntimeException if the injector encounters an error while
     *  providing an instance. For example, an injectable member may throw
     *  an exception which the injector may wrap and propagate.

I may also add an example of using checked exceptions (by defining your own 
interface
instead of using Provider<T>).

Original comment by crazybob...@gmail.com on 23 Jun 2009 at 10:47

[GoogleCodeExporter]

Hey Bob, I agree throwing RuntimeException is effectively the same as throwing 
as subclass thereof so I think what you suggest is entirely appropriate!

:)

cheers!

p.s
   Is this issue now closed?

Original comment by Larry.Ca...@gmail.com on 23 Jun 2009 at 11:14

[GoogleCodeExporter]

Thanks, Larry. I just checked in what I think is a slightly clearer wording. 
Please
review (just the .java files): 
http://code.google.com/p/atinject/source/detail?r=4

Original comment by crazybob...@gmail.com on 23 Jun 2009 at 11:17

• Changed state: Fixed

[GoogleCodeExporter]

looks good! I'm sanguine!

:D

Original comment by Larry.Ca...@gmail.com on 23 Jun 2009 at 11:38