commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Theuns Cloete (JIRA)" <j...@apache.org>
Subject [jira] Commented: (NET-278) FTPClient.disconnect() shouldn't throw IOException
Date Fri, 22 May 2009 09:37:45 GMT

    [ https://issues.apache.org/jira/browse/NET-278?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12712003#action_12712003
] 

Theuns Cloete commented on NET-278:
-----------------------------------

I agree with Sebb that throwing an excpeption is a good idea, because then you can log it.
I do however have a different problem with the disconnect() methods of commons-net:

For easy reference, the SocketClient.disconnect() and SocketClient._connectAction() methods
are given:
public void disconnect() throws IOException
{
        if (_socket_ != null) _socket_.close();
        if (_input_ != null) _input_.close();
        if (_output_ != null) _output_.close();
        if (_socket_ != null) _socket_ = null;
        _input_ = null;
        _output_ = null;
}

and

protected void _connectAction_() throws IOException
{
        _socket_.setSoTimeout(_timeout_);
        _input_ = _socket_.getInputStream();
        _output_ = _socket_.getOutputStream();
}

>From the Java 6 SDK, the _socket_.close() method is also responsible for closing the input
and output streams. But if _socket_.close() throws an IOException before the streams could
be closed, the streams will remain open and they will also not be set to null. We need a way
to ensure that the input and output streams are also closed. There are various ways to achieve
this:

Proposal 1:
Implement a public SocketClient.paranoidDisconnect() method that makes sure the _socket_,
_input_ and _output_ are closed:
    public void paranoidDisconnect() {
        try {
            this.disconnect();
        }
        catch (IOException ioe) {
            // the first thing that SocketClient.disconnect() does is to close the _socket_
            // but if that fails, we have to manually close the _input_ and _output_ streams
            if (this._input_ != null) {
                try {
                    this._input_.close();
                }
                catch (IOException ioe2) {
                }
            }

            if (this._output_ != null) {
                try {
                    this._output_.close();
                }
                catch (IOException ioe3) {
                }
            }
        }
        finally {
            this._socket_ = null;
            this._input_ = null;
            this._output_ = null;
        }
    }


Proposal 2:
Expose the _socket_, _input_ and _output_ with getter methods, handing the responsibility
over to the calling application. TelnetClient already exposes the _input_ and _output_ streams
with getInputStream() and getOutputStream() methods respectively.

> FTPClient.disconnect() shouldn't throw IOException
> --------------------------------------------------
>
>                 Key: NET-278
>                 URL: https://issues.apache.org/jira/browse/NET-278
>             Project: Commons Net
>          Issue Type: Improvement
>    Affects Versions: 2.0
>         Environment: All
>            Reporter: Raffaele Sgarro
>            Priority: Minor
>   Original Estimate: 0.08h
>  Remaining Estimate: 0.08h
>
> FTPClient.disconnect() shouldn't throw IOExceptions because it is typically placed in
a finally block and it doesn't make much sense to
> try {
> client.disconnect()
> } catch (IOException e) {
> // You can't actually do anything
> }
> What is the purpose of such an exception if nobody can use it? There's nothing we can
do if the client couldn't disconnect... You always usa a catch block with a /*do nothing*/
in your samples, so I think it's only an elegant thing to have a try block in a finally block...

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message