commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vincent Bories-Azeau (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (NET-596) NullPointerException when disconnecting TelnetClient twice with JDK 7
Date Tue, 05 Jul 2016 19:45:11 GMT

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

Vincent Bories-Azeau edited comment on NET-596 at 7/5/16 7:44 PM:
------------------------------------------------------------------

I would set the {{\_output\_}} property to {{null}} in a {{finally}} block, to ensure we don't
close twice a stream whose first close has failed:
{noformat}
void _closeOutputStream() throws IOException
{
    if (_output_ != null) {
        try {
            _output_.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            _output_ = null;
        }
    }
}
{noformat}

For the second fix you proposed, which is not related to the issue I created, a {{try-finally}}
block must also surround each {{close}} call, to ensure we attempt to close everything properly,
like this:
{noformat}
public void disconnect() throws IOException
{
    if (__input != null) {
        try {
            __input.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            __input = null;
        }
    }
    if (__output != null) {
        try {
            __output.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            __output = null;
        }
    }
    super.disconnect();
}
{noformat}

To me, I would catch IOExceptions and hide them. I consider at this moment that, whatever
happens, the client must be closed. But, whatever the final policy is, the most important
is to keep the client in a reusable state I guess. As I mentioned, the {{SocketClient.disconnect}}
method is a good example. The doc comments shall also be updated to explain clearly when exceptions
may be thrown, and the behaviour of consecutive calls.
Thanks for your answer!


was (Author: vicente69):
I would set the {{\_output\_}} property to {{null}} in a {{finally}} block, to ensure we don't
close twice a stream whose first close has failed:
{noformat}
void _closeOutputStream() throws IOException
{
    if (_output_ != null) {
        try {
            _output_.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            _output_ = null;
        }
    }
}
{noformat}

For the second fix you proposed, which is not related to the issue I created, a {{try-finally}}
block must also surround each {{close}} call, to ensure we attempt to close everything properly,
like this:
{noformat}
public void disconnect() throws IOException
{
    if (__input != null) {
        try {
            __input.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            __input = null;
        }
    }
    if (__output != null) {
        try {
            __output.close();
        } catch (IOException e) {
            // Shall this exception be ignored, or let it be managed by the caller?
        } finally {
            // Whatever happens, the stream is considered closed.
            __output = null;
        }
    }
    super.disconnect();
}
{noformat}

To me, I would catch IOExceptions and hide them. I consider at this moment that, whatever
happens, the client must be closed. But, whatever the final policy is, the most important
is to keep the client in a reusable state I guess. As I mentioned, the {{SocketClient.disconnect}}
method is a good example.
Thanks for your answer!

> NullPointerException when disconnecting TelnetClient twice with JDK 7
> ---------------------------------------------------------------------
>
>                 Key: NET-596
>                 URL: https://issues.apache.org/jira/browse/NET-596
>             Project: Commons Net
>          Issue Type: Bug
>          Components: Telnet
>    Affects Versions: 3.5
>         Environment: Both telnet client & server with JDK 7.
> Bug not reproduced with JDK 8 (there's another bug with this JDK though, as mentioned
in the description).
>            Reporter: Vincent Bories-Azeau
>
> When using the TelnetClient class, a {{NullPointerException}} may occur when calling
the {{disconnect}} method twice, in the {{_closeOutputStream}} method called under the hood,
if the Telnet connection is lost (for instance, server is hardly shut down).
> 1. The first call to {{disconnect}} resets completely the TelnetClient instance.
> 2. The second call to {{disconnect}} leads to the NPE exception, because the {{\_output\_}}
property is {{null}}, in the {{_closeOutputStream}} method.
> *NOTE: the NPE does not occur with JDK 8, because, the first call to {{disconnect}} throws
an I/O exception (socket is closed), leaving the TelnetClient instance with a non-null {{\_output\_}}
property. Then a second call to disconnect does not throw a NPE. It seems the JDK 8 behaves
differently when a client socket loses a connection. So there is also a bug with JDK 8, as
disconnection shall close quietly resources without an I/O exception, and without leaving
non-null resources, and then disconnect the client socket. The {{SocketClient.disconnect}}
is a good implementation to start with.*
> The problem is that the TelnetOutputStream class closes the Socket output stream under
the hood, but doesn't check if it is null and doesn't reset it to null once done. _The implementation
of the TelnetOutputStream is quite strange, as there is a cycling dependency between this
class and the TelnetClient class. The {{TelnetClient}} class shall handle itself the close
of its internal resources, and disconnect the client socket. But this responsibility is delegates
to the TelnetOutputStream._
> Here's the stack trace of the NPE exception:
> {quote}
> |java.lang.NullPointerException
>      at org.apache.commons.net.telnet.TelnetClient._closeOutputStream(TelnetClient.java:83)
>       at org.apache.commons.net.telnet.TelnetOutputStream.close(TelnetOutputStream.java:163)
>       at org.apache.commons.net.telnet.TelnetClient.disconnect(TelnetClient.java:124)
> {quote}
> A way to workaround this bug, is to always check if the {{TelnetClient}} instance is
connected, before calling the {{disconnect}} method.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message