[net] Unit tests fail for commons-net-1.4.1 with NullPointerException

• Key: NET-13
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Joshua Nichols

Environment

Operating System: Linux
Platform: PC

Description

I'm working on updating the commons-net package for Gentoo to the most recent
version, 1.4.1, and ran into a problem running the unit tests:

[junit] Testsuite: org.apache.commons.net.time.TimeTCPClientTest
[junit] Tests run: 2, Failures: 0, Errors: 1, Time elapsed: 1.093 sec

[junit] Testcase: testInitial took 0.009 sec
[junit] Testcase: testCompareTimes took 1.074 sec
[junit] Caused an ERROR
[junit] null
[junit] java.lang.NullPointerException
[junit] at
org.apache.commons.net.SocketClient.disconnect(SocketClient.java:266)
[junit] at
org.apache.commons.net.time.TimeTCPClientTest.testCompareTimes(TimeTCPClientTest.java:133)

Upon further investigation into SocketClient, I found that disconnect() tried to
close a few objects, without checking that they weren't null. Adding a simple
check for null is enough to get the tests to pass.

Comments

[Joshua Nichols]

Created an attachment (id=17250)
commons-net-1.4.1-socketclient.patch

This patch solves the problem. I'm going to go ahead with packaging 1.4.1 using
this patch, unless there's some overwhelming reason not to.

[Daniel Savarese]

disconnect should not be called on a SocketClient instance that has not
been successfully connected. If there's a problem, it would be
with the unit test, not SocketClient. In other words, the test code should be
if(client.isConnected())
client.disconnect();
instead of
client.disconnect();

However, if you do that, then the unit test doesn't fail when a connection
is not established. So you could write
if(client.isConnected())
client.disconnect();
else
throw SomeAppropriateException("Connection failed.");

But since client.disconnect() already throws an exception, I have to assume
the test writer opted for the shorter
client.disconnect();

In short, the test is supposed to fail if it can't connect.

[Daniel Savarese]

(In reply to comment #1)
> This patch solves the problem. I'm going to go ahead with packaging 1.4.1 using
> this patch, unless there's some overwhelming reason not to.

I don't mean to speak for the rest of the Commons Net developers, but I think
we'd rather you package it by bypassing the unit tests during packaging.
Applying your patch may cause more bug reports from people who call disconnect()
improperly. With the patch, they'll have no indication they did something wrong.
We can discuss this further on commons-dev.

[Joshua Nichols]

(In reply to comment #2)
> disconnect should not be called on a SocketClient instance that has not
> been successfully connected. If there's a problem, it would be
> with the unit test, not SocketClient. In other words, the test code should be
> if(client.isConnected())
> client.disconnect();
> instead of
> client.disconnect();
> However, if you do that, then the unit test doesn't fail when a connection
> is not established. So you could write
> if(client.isConnected())
> client.disconnect();
> else
> throw SomeAppropriateException("Connection failed.");
>
> But since client.disconnect() already throws an exception, I have to assume
> the test writer opted for the shorter
> client.disconnect();

Fair enough. However, I think that some exception should be thrown (maybe
IllegalStateException?) that indicates that the object is in an invalid state,
instead of letting a NullPointerException occur.

>
> In short, the test is supposed to fail if it can't connect.
I can accept that the test would fail if it can't connect. However, it really
shouldn't be because of a NullPointerException.

(In reply to comment #3)
> I don't mean to speak for the rest of the Commons Net developers, but I think
> we'd rather you package it by bypassing the unit tests during packaging.

I concurr about not including the patch. We actually prefer to keep packages as
close to upstream as possible.

> Applying your patch may cause more bug reports from people who call disconnect()
> improperly. With the patch, they'll have no indication they did something wrong.
> We can discuss this further on commons-dev.

The current code doesn't give much better of an indication that something went
wrong, so perhaps throwing an exception, like I mentioned above, is more
appropriate?

[Daniel Savarese]

I changed the test to check isConnected before calling disconnect. My
original statement about what the original test writer probably intended
was incorrect. connect() throws an exception when it fails. The lack
of any catch block indicates the test writer intended for the original
exception to pass through. However, in that context--by placing the
disconnect in a finally block--it is incorrect for the test to call
disconnect without first checking isConnected. Sorry for my original
misdiagnosis.

With regard to SocketClient, I have no objection to making disconnect throw
an IllegalStateException if that is where the consensus of the Commons Net
user and developer community has moved. However, that would be an
enhancement request and should be discussed on the commons-dev mailing
list first. The reason an IllegalStateException is not thrown is because
the library was designed with the philosophy of not checking every possible
API misuse and throwing an exception in each case. Doing so would have
littered the code with a lot of if then else throw sequences, making the
API throw even more exceptions than it already does. The consensus in the
past has been to not check every possible mistake in API use. However,
that consensus can be retested on commons-dev.

[Steve Cohen]

My only comment here is that you've patched the head which is not the same as
patching version 1.4.1. This is exactly the right thing to have done, I am not
criticizing at all but I just want to clarify this point since the summary
mentions 1.4.1 specifically.

Version 1.4.1 was handled as a BRANCH off 1.4.0, rather than the usual release
off the latest HEAD. I did it this way because we were getting frequent
complaints about our binary packages being incompatible with JDK 1.3, and I was
pressed for time and it was easier. 1.4.1 fixes the 1.3 JDK problem and only
that problem. It doesn't even include changes made to the HEAD since 1.4.0
before the release of 1.4.1.

I mainly bring this up because we're probably approaching the point where we
should think about making a proper 1.4.2 release that releases all the fixed
defects we've accumulated over the past 7 or 8 months.