commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel F. Savarese" <...@savarese.org>
Subject Re: [net][patch] Added more robust SocketClient setup/cleanu
Date Mon, 22 Sep 2003 20:38:53 GMT

In message <858yoqyy3z.fsf@brekke.org>, Jeffrey D. Brekke writes:
>Applying the patch and cutting a RC1 gets a +1 from me.  
>
>I'd say after Wed./RC1, leave the RC1 out for a week, then if no
>issues show up do a 1.1 release?

I didn't get around to applying the changes suggested by the patch
until today.  After making the changes, I found myself writing a
commit message that explained the reason for the changes and why
they weren't applied to classes like FTPClient and NNTPClient.
In the process of writing the message, I aborted the commit to
revisit why we need these changes.

The isConnected() and isOpen() checks only appear to be okay because
the classes to which they apply perform only one action.  However,
imagine if we checked isConnected() in every method of FTPClient.
I'd rather have a consistent policy toward which programming mistakes
we protect against and which we don't.  If the reason for implementing
protective measure applies to all of the client classes, but the change
itself is not suitable for all of the classes, I'd rather not implement
it for any of the classes and delegate the responsibility to the API
user.

The reasoning for the null checks in close() made sense when explained,
but after applying the changes they didn't make sense to me.  First, the
proposed change for SocketClient swallows exceptions we want reported to
the programmer.  So I changed the patch to keep the null checks, but
rethrow the exception.  However, the way close() is implemented originally,
a socket or stream reference cannot ever be null if close() throws an
exception (all of the null assignments are made after the close() calls).
In other words, if you call close() twice in succession and the first call fails,
the second call will throw an exception other than NullPointerException.
If the first call succeeds then the second call will indeed result in the
accessing of null references.  However, programmers shouldn't be writing code
that allows a close() or disconnect() to be called in a finally block after a
successful call to close() or disconnect().  They should check in the
finally block whether the client is connected or open with isConnected()
or isOpen().  By applying the suggested changes, we encourage careless
programming.  Now, don't get me wrong, the Commons Net API is by no means
a model of perfection and it could really use an overhaul because it makes
dealing with exceptions messy (e.g., src/java/examples/ftp.java is evidence
of this).  So it's not like I'm saying we don't have things we need to do
to make programming with Commons Net cleaner.

In any case, I've talked my way back to being -1 on the changes.  If
I wasn't very clear in my explanation of why, let me know and I'll
clarify.  I suggest we move forward with a 1.1 RC1 vote as Jeffrey
proposed and resolve any further debate about this patch after the
1.1 release.

daniel



Mime
View raw message