[SSHD-85] Tests & Candidate fix #33

bkuker · 2017-06-22T15:45:39Z

SSHD-85 is a bug that occurs on Windows where the last few bytes of data sent through a port forward can be lost when the server at the far end of the port forward sends the data and immediately closes the socket.

An example of this server behavior would be HTTP 1.0 with the Connection: close header. The server sends the data, and then closes the socket to indicate that it is done sending data. The server does not send a Content-Length header or similar. This is a stupid way to do it, all modern http servers do better, but there are still old embedded systems that work this way, and it does impact my use case.

The test cases include some sleeps on the client end. This seems to be a race condition that occurs possibly one time in a hundred or more, but these sleeps cause it to happen every single time.
The test cases include two controls: JSCH as the client

During testing I did verify that the test server did send all data to the ssh client by examining the buffers in the ssh client and seeing all of the data. I then verified that all of the data was transferred from the ssh client to the ssh server by examining the buffers in the sshd server. The sshd server does write all of that data to the socket as far as I can tell, but on Windows some still goes missing at the end! I also verified that on Local port forwards the error does occur with Apache SSHD as the client, and does NOT occur with JSCH as the client. I would like to try that the other way around with Apache SSH Client and another server, but the pickings for Java SSH servers are slim, and this one is the best!

I am currently trying to understand WHY the fix (basically one line, getSocket().shutdownOutput()) fixes the problem. I am submitting this PR now in hopes that someone with a better understanding will point out why it is the correct fix and accept it. All I can say at the moment is that it works.

Also includes a fix for a test in PortForwardingLoadTest which was failing for me before I started investigating. Frankly it looks like it never worked; I would guess that there was some copy-paste mistake in the past when it got updated to use try-with-resources.

I am ... very motivated to get this fixed so I do not have to maintain my own private branch of sshd, so please ask questions, make suggestions, if you think I am totally crazy help me prove that I am not!

lgoldstein

In general, in order for me to actually test the code I need it to conform to the project style. Please make sure you run a mvn clean install before re-posting the changes. In general here is what seems to be missing:

Standard license header
Standard class javadoc format
We no longer use final modifier
All our test methods are called testXXX
All our test classes extend BaseTestSupport and call assertXXX directly instead of Assert.assertXXX

Please make the necessary changes and re-post once mvn clean install passed so I can start examining it.

lgoldstein · 2017-06-22T16:33:40Z

sshd-core/src/test/java/org/apache/sshd/common/forward/PortForwardingLoadTest.java

@@ -215,6 +215,7 @@ public void run() {
                                try (InputStream sockIn = s.getInputStream();
                                     ByteArrayOutputStream baos = new ByteArrayOutputStream()) {

+                                	//Reag a payload worth of data, save it to baos
                                    while (baos.size() < payload.length()) {


Typo error (I assume) Read...

Please align the comment indentation

lgoldstein · 2017-06-22T16:34:39Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

@@ -0,0 +1,227 @@
+package org.apache.sshd.common.forward.sshd85;


Please attach the standard license header

lgoldstein · 2017-06-22T16:35:11Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+ * close the TCP connection to indicate that all data has been sent.
+ *
+ * This test implements TWO methods to connect to and read data from that
+ * server.


Please format this comment according to our standards

lgoldstein · 2017-06-22T16:38:30Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+ * @author bkuker
+ *
+ */
+public abstract class AbstractServerCloseTest {


All out tests extend BaseTestSupport class

lgoldstein · 2017-06-22T16:38:32Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+	private final String PAYLOAD;
+	private AsynchronousServerSocketChannel testServerSock;
+
+	public AbstractServerCloseTest() {


abstract class constructor should be protected

lgoldstein · 2017-06-22T16:38:36Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+	 */
+	@Before
+	public void startTestServer() throws Exception {
+		final InetSocketAddress sockAddr = new InetSocketAddress("localhost", TEST_SERVER_PORT);


no need to declare it final

lgoldstein · 2017-06-22T16:39:12Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+					@Override
+					public void completed(final AsynchronousSocketChannel sockChannel,
+							final AsynchronousServerSocketChannel serverSock) {
+						// a connection is accepted, start to accept next


Please remove final modifier

lgoldstein · 2017-06-22T16:39:25Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+						// a connection is accepted, start to accept next
+						// connection
+						serverSock.accept(serverSock, this);
+						final ByteBuffer buf = ByteBuffer.wrap(PAYLOAD.getBytes());


Please remove final modifier

lgoldstein · 2017-06-22T16:40:39Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+			try {
+				final int read2 = s.getInputStream().read(b2);
+				log.info("Got {} bytes from the server: {}", read2, new String(b2, 0, read2));
+				Assert.assertEquals(PAYLOAD, new String(b1, 0, read1) + new String(b2, 0, read2));


We use direct assertXXX calls (inherited from BaseTestSupport)

bkuker · 2017-06-22T16:53:24Z

Will do thanks! I had just copied these tests from my internal branch and had not worried about style yet...
Quick question "We no longer use final modifier" Like... Ever? I'll happily remove all instances from my code, I am just so used to modern style guidelines that say the exact opposite!

lgoldstein · 2017-06-22T17:05:28Z

modern style guidelines that say the exact opposite

Our (current) opinion is that it makes the code cluttered with un-necessary final modifiers all over the place. Java 8 is very good in deciding which variables are effectively final, so no need to state this explicitly. As far as other uses of final that style guidelines refer to - I would be happy to discuss them, but not just right now. Basically, unless some special case (none of which currently comes to mind) I would rather not use final (it has some debugging ramifications as well...)

lgoldstein · 2017-06-22T17:08:25Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerApacheClient.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.forward.sshd85;


No need for the sshd85 package - org.apache.sshd.common.forward is good enough

lgoldstein · 2017-06-22T17:09:48Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerApacheClient.java

+		log.info("SSHD Running on port {}", server.getPort());
+	}
+
+	@AfterClass


Please use some reasonable timeout for the await to avoid stuck servers,

lgoldstein · 2017-06-22T17:11:19Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerApacheClient.java

+	@Override
+	protected int startLocalPF() throws Exception {
+		final SshdSocketAddress remote = new SshdSocketAddress("localhost", 0);
+		final SshdSocketAddress local = new SshdSocketAddress("localhost", TEST_SERVER_PORT);


Don't use localhost - use TEST_LOCALHOST from BaseTestSuppot

lgoldstein · 2017-06-22T17:12:29Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerJSchClient.java

+public class ApacheServerJSchClient extends AbstractServerCloseTest {
+	private static final Logger log = LoggerFactory.getLogger(ApacheServerJSchClient.class);
+
+	private static final int SSH_SERVER_PORT = 1123;


Please don't use static ports - what if this port is used by some service on the user's host

lgoldstein · 2017-06-22T17:14:06Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/NoServerNoClient.java

+		return TEST_SERVER_PORT;
+	}
+
+}


Superfluous empty line

lgoldstein · 2017-06-22T17:14:41Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/NoServerNoClient.java

+ */
+package org.apache.sshd.common.forward.sshd85;
+
+public class NoServerNoClient extends AbstractServerCloseTest {


Missing explicit public no-args constructor

lgoldstein · 2017-06-22T17:15:23Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerJSchClient.java

+
+	@Override
+	protected int startRemotePF() throws Exception {
+		port++;


Use AtomicInteger.incrementAndGet()

Actually I had to change that to use dynamically chosen free ports instead.

lgoldstein · 2017-06-22T17:16:44Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerJSchClient.java

+	@Before
+	public void createClient() throws Exception {
+		log.info("Creating SSH Client...");
+		JSch client = new JSch();


Please call JSchLogger.init() in the @BeforeClass method

lgoldstein · 2017-06-22T17:17:24Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerJSchClient.java

+
+	@AfterClass
+	public static void stopServer() throws IOException {
+		server.close(true).await();


Please add a reasonable await timeout to avoid stuck servers

lgoldstein · 2017-06-22T17:20:17Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/AbstractServerCloseTest.java

+ * @author bkuker
+ *
+ */
+public abstract class AbstractServerCloseTest {


abstract classes from which other tests are derived are called XXXTestSupport

lgoldstein · 2017-06-22T17:21:08Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerApacheClient.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ApacheServerApacheClient extends AbstractServerCloseTest {


Test classes are called XXXTest and have a @FixMethodOrder(MethodSorters.NAME_ASCENDING annotaton

lgoldstein · 2017-06-22T17:22:05Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/ApacheServerJSchClient.java

+import com.jcraft.jsch.JSch;
+import com.jcraft.jsch.Session;
+
+public class ApacheServerJSchClient extends AbstractServerCloseTest {


See comment regarding test class name and required annotation

lgoldstein · 2017-06-22T17:22:16Z

sshd-core/src/test/java/org/apache/sshd/common/forward/sshd85/NoServerNoClient.java

+ */
+package org.apache.sshd.common.forward.sshd85;
+
+public class NoServerNoClient extends AbstractServerCloseTest {


See comment regarding test class name and required annotation

bkuker · 2017-06-22T17:27:37Z

@lgoldstein Before I kill you with 1000 more style violations, can you direct me to the current version of those guidelines? The web page https://mina.apache.org/sshd-project/sources.html is horribly out of date, and the "contributing" link just asks for money, not code!

bkuker · 2017-06-22T18:06:09Z

Apologies for so many commits - I am not able to upgrade Maven on my Windows desktop where the bug exists, so I have to run checkstyle on the Linux desktop! I am getting dizzy spinning my chair from one to the other.

bkuker · 2017-06-22T18:36:09Z

Thanks so much for your feedback; I'd been so deep in debugging the past few days I was worried less about the style. Also I did not mean to imply anything negative about the "final" style, I have just been getting used to doing it more often.
Rat and checkstyle now pass, and I think I have addressed all of your comments so far. I have to admit I am still not sure where to find the style guidelines, but I'll be happy to keep working to fix things up.

(BTW at the moment fixing this falls under "my job" so I'll be very quick to fix anything you ask, please don't take that for impatience or demanding responses from you immediately)

lgoldstein · 2017-06-22T19:38:37Z

Thanks for all your effort, my comments may seem short/terse but they are in no way a sign of impatience - I appreciate your willingness to adhere to the project standards. Many programmers discount style as being petty, but since this is an open-source project, it is helpful to have the source code have a standard look-and-feel so one can find one's way easily around it. I apologize for not having more time to discuss or write a document explaining the style choices and why we chose them - but as you may have surmised, my involvement with this project is purely on a voluntary basis - for which my day job does not leave too much time. All I can recommend at the moment is to look at other existing test classes and adopt the style in them.

bkuker · 2017-06-22T19:44:30Z

No problem and no worries, and I totally agree (Well I may have a different opinion on tabs and final but it is more important to conform to contribute!)

I'll have another look through later on, I think the tests are a lot better then before your feedback.

I also want to take some time to better understand WHY the one line fix makes a difference, because I fee like the behavior should be the same with or without it.

bkuker · 2017-06-23T12:46:37Z

Thanks for the updates, every one is an improvement to the tests. They still pass, which is good, and they still fail without the fix, which is great.

I am not sure from your branch if you intended to remove ApacheServerJSchClientTest and NoServerNoClientTest.

IMHO the JSCH one should still be included. I actually had to write it in order to narrow down the problem.

The NoServerNoClientTest... I see it as a test to ensure that the Test Server and Test Client work properly, and confirmation that yes, TCP works that way and you should be able to expect to receive all the data. I did have to run it and see it pass ten thousand times before I was certain enough to re-open the ticket :)

lgoldstein · 2017-06-24T14:53:19Z

sshd-core/src/test/java/org/apache/sshd/common/forward/PortForwardingLoadTest.java

+                                    try (OutputStream sockOut = s.getOutputStream()) {
+                                        sockOut.write(baos.toByteArray(), 0, payload.length());
+                                        sockOut.flush();
+                                        sockOut.close();


Why the change here ?

I probably should not have mixed this in, it is unrelated to SSHD-85, sorry. This test did not pass on the unmodified code base , it seems logically wrong, so I fixed it.

lgoldstein · 2017-06-24T14:56:59Z

I have added all the tests from this PR to my branch and surprisingly, the previously failing test now succeeds - I am not comfortable with this as it seems that the tests affect each other. I will run some more tests to make sure that is not the case...

bkuker · 2017-06-24T15:07:02Z

I suspect that with so many minor formatting, renaming, and small cleanups and fixes something may have been broken.. I will go through everything thoroughly and make sure each test still fails as expected without the fix, and passes as expected with the fix.

I will also remove the unrelated fix to PortForwardingLoadTest from this PR. I will look further into what I believe is a bug in that test, make sure it really is a bug, and track it as a different ticket as I should have from the beginning!

lgoldstein · 2017-06-24T15:23:41Z

I updated the branch - everything passes - I am still running some tests to be doubly sure - including running tests on Linux (Fedora 22 in this case). If they all pass I will merge the branch

bkuker · 2017-06-25T14:00:39Z

FInal version was completed and merged by @lgoldstein
Thank you so much for your help!

bkuker added 3 commits June 21, 2017 14:10

Tests for SSHD-85

e6e400b

The Fix?

fe92dc6

This test failed before my changes. It made no sense as it was.

5842c85

lgoldstein suggested changes Jun 22, 2017

View reviewed changes

lgoldstein reviewed Jun 22, 2017

View reviewed changes

bkuker changed the title ~~Tests & Candidate fix for SSHD-85.~~ [SSHD-85] Tests & Candidate fix Jun 22, 2017

Style & Code Review Fixes.

09f5ae0

bkuker force-pushed the sshd-85 branch from fbe3c7c to 09f5ae0 Compare June 22, 2017 18:26

bkuker and others added 2 commits June 23, 2017 08:16

Some improvements thanks to lgoldstein

69b642b

Getting a little techy with checkstyle...

c93a520

lgoldstein reviewed Jun 24, 2017

View reviewed changes

bkuker closed this Jun 25, 2017

		@@ -0,0 +1,227 @@
		package org.apache.sshd.common.forward.sshd85;

[SSHD-85] Tests & Candidate fix #33

[SSHD-85] Tests & Candidate fix #33

Conversation

bkuker commented Jun 22, 2017

lgoldstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkuker commented Jun 22, 2017

lgoldstein commented Jun 22, 2017

lgoldstein Jun 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkuker commented Jun 22, 2017

bkuker commented Jun 22, 2017

bkuker commented Jun 22, 2017

lgoldstein commented Jun 22, 2017

bkuker commented Jun 22, 2017

bkuker commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoldstein commented Jun 24, 2017 • edited

bkuker commented Jun 24, 2017

lgoldstein commented Jun 24, 2017

bkuker commented Jun 25, 2017

lgoldstein Jun 22, 2017 •

edited

lgoldstein commented Jun 24, 2017 •

edited