Re: [Proxool-developer] ProxyFactory.buildConnection() - code review

[Bill H. <bi...@lo...>]

Bertrand,

On Thu, 2004-03-11 at 09:45, Bertrand Renuart wrote:

> I noticed the 'new ProxyConnection()' could throw an SQLException. If
> this happens, the *real* connection is not closed although it should
> probably be...

Well spotted.

> On the other hand, I had a look at why this constructor could throw
> the exception... And it can do so only if the 'realConnection' given
> as argument is null - there is a test at the end of the constructor...
> ==> Wondering when this could happen?

Agreed. It does seem a bit paranoid to test for null. The delegate
driver should either return a connection of throw an exception. 

> - if it is because we failed to obtain a connection from the
> DriverManager, then this should be tested in
> ProxyFactory.buildConnection (code snippet above);

Yep. I don't see why we don't do a test straight after asking for one. 

> - if it is because part of the ProxyConnection constructor will set
> the field to null if a problem occurs (don't think so), then ok, an
> SQLException could be thrown by the constructor - but then, the *real*
> connection should be released by the ProxyFactory...

That doesn't happen. The only reason why new ProxyConnection() will
throw an SQLException is if tthe real connection is null, and I think we
should test for that before getting to the stage of building the proxy
connection.

> (is it clear ?)

Absolutely.

> If the latter is true, then the ProxyFactory.buildConnection() should
> be modified as follows:
[snip]
>             try {
>                 // Ooops - unable to build the proxy - close the
> realConnection
>                 if(realConnection!=null) realConnection.close();
>             }
>             catch (SQLException e) {
>                 // Forget it - not much todo about it :(
>             }
[snip]

Well, the latter isn't true. But I still think this try/catch is worth
implementing. I don't think we need to catch SQLException (we may well
remove it from the new ProxyConnection signature) but we should
certainly catch RuntimeExceptions and make sure we close the real
connection. 

- Bill