Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Message-ID: <314543268.1242985065564.JavaMail.jira@brutus>
Date: Fri, 22 May 2009 02:37:45 -0700 (PDT)
From: "Theuns Cloete (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Subject: [jira] Commented: (NET-278) FTPClient.disconnect() shouldn't throw
 IOException
In-Reply-To: <1147592097.1242161505729.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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.