Re: Maybe Cursor should also extends Closable

[Radovan Semancik <ra...@evolveum.com>]

+1 for going the Closeable way.

Close operation can end up with an error (e.g. timeout). That cannot be 
avoided. And for me it is much easier (and much less error-prone) to 
handle a checked exception than to do a complex sequence of close() and 
then check for isClosed() or isConnected() and similar and then trying 
to figure out what really was the root cause because the original 
exception got eaten somewhere deep in the code. Having close() that 
throws IOException will be a nudge to all the clients to do at least 
some error handling.

Of course, the problem is that it is IOException and not LdapException. 
One of the solution would be to make LdapException subclass of 
IOException? That obviously does not fit 100% well, as some subclasses 
of LdapException and more related to parsing than to IO. But I think 
that it is still somehow acceptable. Another (and perhaps cleaner) would 
be to throw IO exceptions that happen during the close directly as IO 
exceptions without wrapping them to LdapException. And that might be the 
right thing to also for all other operations: if the operation dies on 
network condition (e.g. TCP timeout), throw original IO exception. If it 
dies on LDAP condition (e.g. LDAP error code 49) throw LdapException. 
That would solve the connection being Closeable. Now for cursor there is 
still this tricky part that the close() on cursor might want to abort an 
operation and therefore it may produce LDAP errors. So maybe the best 
thing to do would to do both: make LdapException subclass of IOException 
and pass low-level IO exceptions without wrapping them to LdapException.

This is how I would do it. But maybe the API has some other philosophy 
when dealing with errors? If there is one, I must have missed it so I 
will be very grateful for anyone to point my nose in the right 
direction. However, I really think one of the big drawbacks of the API 
is error handling in general. Especially in the schema processing part, 
but there are obviously also other parts that could use some 
improvement. We obviously cannot change everything in a day. So 
gradually improving the status quo seems to be a good way to take.

-- 
Radovan Semancik
Software Architect
evolveum.com



On 01/29/2016 10:53 AM, Emmanuel Lécharny wrote:
> Le 29/01/16 10:06, Maxim Solodovnik a écrit :
>> sorry for cross-posting, but I have no response in user list
>> Hopefully this list will fit better :)
> Sorry for having been mute :/
>
> Implementing Closeable was suggest in december 2011 by Stefan.
>
> This have a huge impact on the code, as the current close() method does
> not throw any exception atm, and we will have to take care of the
> IOException all over. OTOH, this is not such a problem.
>
>
>
> guys, wdyt ?
>

Re: Maybe Cursor should also extends Closable

[Emmanuel Lécharny <el...@gmail.com>]

Le 02/02/16 10:52, Radovan Semancik a écrit :
> +1 for going the Closeable way.
>
> Close operation can end up with an error (e.g. timeout). That cannot
> be avoided. And for me it is much easier (and much less error-prone)
> to handle a checked exception than to do a complex sequence of close()
> and then check for isClosed() or isConnected() and similar and then
> trying to figure out what really was the root cause because the
> original exception got eaten somewhere deep in the code. Having
> close() that throws IOException will be a nudge to all the clients to
> do at least some error handling.
>
> Of course, the problem is that it is IOException and not
> LdapException. One of the solution would be to make LdapException
> subclass of IOException? That obviously does not fit 100% well, as
> some subclasses of LdapException and more related to parsing than to
> IO. But I think that it is still somehow acceptable. Another (and
> perhaps cleaner) would be to throw IO exceptions that happen during
> the close directly as IO exceptions without wrapping them to
> LdapException. And that might be the right thing to also for all other
> operations: if the operation dies on network condition (e.g. TCP
> timeout), throw original IO exception. If it dies on LDAP condition
> (e.g. LDAP error code 49) throw LdapException. That would solve the
> connection being Closeable. Now for cursor there is still this tricky
> part that the close() on cursor might want to abort an operation and
> therefore it may produce LDAP errors. So maybe the best thing to do
> would to do both: make LdapException subclass of IOException and pass
> low-level IO exceptions without wrapping them to LdapException.
>
> This is how I would do it. But maybe the API has some other philosophy
> when dealing with errors? If there is one, I must have missed it so I
> will be very grateful for anyone to point my nose in the right
> direction. However, I really think one of the big drawbacks of the API
> is error handling in general. Especially in the schema processing
> part, but there are obviously also other parts that could use some
> improvement. We obviously cannot change everything in a day. So
> gradually improving the status quo seems to be a good way to take.
>
FTR, I have changed the Cursor interface to extends Closeable. After a
few changes, I tested teh API and ApacheDS, it does the job.


It makes no sense for the Cursor close() method to throw a
LdapException, it's way to narrow. Throwing an IoException is just fine.

regarding the exception handling in general, in teh API, yes, we are
aware of it. We have a JIRA opened since 2011 :
https://issues.apache.org/jira/browse/DIRAPI-116?jql=project%20%3D%20DIRAPI%20AND%20resolution%20%3D%20Unresolved%20ORDER%20BY%20priority%20DESC.
This is not an easy thing to fix though, as it impacts a hell lot of
classes, but at least we can start to think about it.

I'd suggest to create another thread to talk about this aspect.

In the mean time, I'm going to commit the Cursor change (still have to
check the impact on Studio).

thanks !