Getting different results at different times when running in threads

There are 3 major things you can improve in the code above:

• Use thread safe collections when modifying them from multiple threads. If the collection doesn't explicitly mention that it's thread-safe, you will need to either grab locks yourself or switch to a thread-safe collection
• Using a Thread.sleep() for synchronization is generally a bad idea. You should use more precise methods of synchronization to avoid flaky behavior. I've chosen to use a CountDownLatch here, you could use other synchronization tools.
• Exceptions thrown inside threads don't propagate up to the main thread but may just bubble up inside that thread and never be noticed. You want to be able to notice exceptions thrown inside the runnable and handle it at the end. I will also modify the code to show you a rough guide how to handle exceptions.

Going to answer your question in bare-bones format and you can hopefully make the adjustments to your code:

public List getVersion(String getVersionJson)        throws InterruptedException, IOException {    // You must use a thread-safe list.    final List outputList = Collections.synchronizedList(new ArrayList());    // This reference is used to keep track of exceptions thrown    // in the runnable.    final AtomicReference<Exception> exceptionReference =        new AtomicReference(null);    // This latch acts as a synchronization barrier so that all threads    // must have put data into outputList before the code will go    // past the latch.await() call    final CountDownLatch latch = new CountDownLatch(jsonArr.length());    for (int j = 0; j < jsonArr.length(); j++) {        new Thread() {            public void run() {                try {                        // Do network I/O stuff                     outputList.add(resultFromNetworkStuff);                     latch.countDown();                } catch (IOException e) {                     // Set the exception for later retrieval                     exceptionReference.compareAndSet(null, e);                }            }                                }.start();    }    // Wait for all the threads to finish before continuing.    try {        latch.await();    } finally {        // Even if we're interrupted, we want to close the connection.        closeSSH();    }    if (exceptionRef.get() != null) {        // Throw any exceptions we see in our threads.        throw exceptionRef.get();    }    return outputList;}

You put your main thread to sleep for some hard coded period of time. And during that time multiple other threads push data in parallel into a shared list. This cannot work by definition.

When your other thread is done within the 20 seconds, all fine. But if not, your method does return whatever the other threads collected so far. And worse, that other threads keeps updating that list which you already returned.

You have to make sure that the inner threads

• are not accessing that result list in that uncontrolled manner (overwriting each other!)

• are all done before returning from this method.

Don't apply some concept in your code because you heard of it without understanding what you are doing. You have not much clue how to work with java threads and what it means using them. Thus study this stuff instead of blindly pushing it into your code. Learn how to use threads using simple examples; instead of pulling them into your production code like this.

Then: your catch block is throwing away error information! That is always a bad idea. Same for your approach of pushing error information as strings in your result list.

Beyond that: read Clean Code by Robert Martin. Your code would benefit much more from that than from you adding threads for no reason.