Inner class calling outer class method

Question

I created a console program that tests report results from JasperReport. It retrieves the list of reports, splits it, and passes each sublist to a new thread. Each thread executes its own reports and compares the results with reference files.

My colleague says multithreading is wrong, but as usual he doesn't explain why. Any hint on what is not correct? He just spouts off something about using an inner class but was not clear, and it's hard to get more details.

This is how the code looks. I've omitted some irrelevant functions (with no side effects anyway). Every local variable is final.

public class ReportTester {
 private class ThreadTest implements Runnable {
 final List<Report> reports;
 final Configuration config;
 public ThreadTest(final List<Report> reports, Configuration config) 
 this.reports = reports;
 this.config = config;
 }
 @Override
 public void run() {
 runTest(this.reports, this.config);
 }
 }
 private final String format = "xml";
 private final String directoryReport = "\var\reports";
 private final JasperRestfulClient restClient = new JasperRestfulClient();
 private final List<Report> reportsToBeTested = restClient.getReports();
 volatile private errors = false; // SIDE EFFECT HERE. Public getter omitted.
 private void runTest(List<Report> reports, Configuration config) {
 for (Report report : reports) {
 try {
 String fileName = getFilePath(directoryReport, report, config);
 restClient.runReport(report.getPath(), format,config, fileName);
 compareWithReference(fileName, report, config);
 }
 catch(Exception ex){
 ex.printStack();
 } 
 }
 }
 public void runTestMultithreading(Configuration config, int numThread){
 ExecutorService es = Executors.newCachedThreadPool();
 List<List<Report>> splitted = splitReports(reportsToBeTested, numThread);
 for (List<Report> reportsOfThread : splitted) {
 ThreadTest thread = new ThreadTest(reportsOfThread, config);
 es.execute(thread);
 }
 es.shutdown();
 es.awaitTermination(8, TimeUnit.HOURS);
 }
}

EDIT: This are the methods omitted. Actually there is a side effect, a boolean variable is assigned to true if there is at least one report that isn't identical with reference. But there is not race conditions, even without synchronization, because it can be only assigned to true, the value assigned doesn't depend on previous value of variable and it is and never read by threads.

private boolean filesAreIdentical(String filenameFirst, String filenameSecond) {
 File file1 = new File(filenameFirst);
 File file2 = new File(filenameSecond);
 if (!file1.exists() || !file2.exists()) {
 return false;
 }
 if (file1.length() != file2.length()) {
 return false;
 }
 InputStream stream1 = null;
 InputStream stream2 = null;
 try {
 stream1 = new FileInputStream(file1);
 stream2 = new FileInputStream(file2);
 final int BUFFSIZE = 1024;
 int read1 = -1;
 int read2 = -1;
 byte buffer1[] = new byte[BUFFSIZE];
 byte buffer2[] = new byte[BUFFSIZE];
 do {
 int offset1 = 0;
 while (offset1 < BUFFSIZE && (read1 = stream1.read(buffer1, offset1, BUFFSIZE - offset1)) >= 0) {
 offset1 += read1;
 }
 int offset2 = 0;
 while (offset2 < BUFFSIZE && (read2 = stream2.read(buffer2, offset2, BUFFSIZE - offset2)) >= 0) {
 offset2 += read2;
 }
 if (offset1 != offset2) {
 return false;
 }
 if (offset1 != BUFFSIZE) {
 Arrays.fill(buffer1, offset1, BUFFSIZE, (byte) 0);
 Arrays.fill(buffer2, offset2, BUFFSIZE, (byte) 0);
 }
 if (!Arrays.equals(buffer1, buffer2)) {
 return false;
 }
 } while (read1 >= 0 && read2 >= 0);
 return read1 < 0 && read2 < 0;
 } catch (IOException e) {
 e.printStackTrace();
 return false;
 } finally {
 try {
 if (stream1 != null) {
 stream1.close();
 }
 } catch (IOException e) {
 e.printStackTrace();
 }
 try {
 if (stream2 != null) {
 stream2.close();
 }
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
}
private List<List<Report>> splitReports(List<Report> original, int number) {
 List<List<Report>> sublists = new ArrayList<List<Report>>(number);
 int reportsPerThread = original.size() / number;
 for (int i = 0; i < number; i++) {
 int start = reportsPerThread * i;
 int stop = (i == number - 1) 
 ? original.size() - 1 
 : reportsPerThread * (i + 1) - 1;
 List<Report> sublist = createSublist(original, start, stop);
 sublists.add(sublist);
 }
 return splits;
}
private List<Report> createSublist(final List<Report> original, int start, int stop) 
{
 List<Report> copy = new ArrayList<>(original.size());
 if (stop > original.size() - 1) {
 stop = original.size() - 1;
 }
 for (int ii = start; ii <= stop; ii++) {
 copy.add(original.get(ii));
 }
 return copy;
}
private void compareWithReference(String filename, Report report, Configuration config) {
 String filenameReference = directoryReport + config.subfolder() + filename;
 if (filesAreIdentical(filename, filenameReference)) {
 System.out.println(filename + " OK");
 } else {
 // SIDE EFFECT
 errors = true;
 System.err.println(filename + " FAILED");
 }
}

This is the the client.

private final String server; //assigned by constructor
protected final CloseableHttpClient httpclient; //assigned by constructor; threadsafe
public void runReport(String url, String destinazione) throws IOException {
 HttpGet httpget = null;
 InputStream instream = null;
 try {
 File file = new File(destinazione);
 file.getParentFile().mkdirs();
 file.createNewFile();
 httpget = new HttpGet(server + url); 
 HttpResponse response = httpclient.execute(httpget);
 writeResponceToFile(response, file);
 }
 finally {
 if (httpget != null) {
 httpget.abort();
 }
 }
}
private void writeResponceToFile(HttpResponse response, File file) throws IOException {
 HttpEntity entity = response.getEntity();
 InputStream instream = null;
 try {
 if (entity != null) {
 instream = entity.getContent();
 BufferedInputStream bis = new BufferedInputStream(instream);
 BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(file));
 int inByte;
 while ((inByte = bis.read()) != -1) {
 bos.write(inByte);
 }
 bis.close();
 bos.close();
 }
 } finally {
 if (instream != null) {
 instream.close();
 }
 }
}

Answer 1

I can only see one 'bug' problem with this code, but there are a few other 'style' and 'simplicity' issues.

reportsDaTestare

What is this? It appears out of nowhere:

for (Report report : reportsDaTestare) {
 ....
}

I am worried that this is a typo for reportsToBeTested as you tanslated the code.... If it is, then you will be repeating all the reports in each thread..... it should be:

for (Report report : reports) {
 ....
}

Style

• Test and class and method names derived from it typically relate to unit testing, like jUnit, etc. By common convention you should not use these method names for anything other than test code.

  Use words like Validate, or Check instead.

• Error handling.... I presume you have removed the actual error handling from this method, because this code will not compile... it is ex.printStackTrace() and not ex.printStack(). I hope the error-handling code you removed is 'better'.

   catch(Exception ex){
   ex.printStack();
   }
  

• ThreadTest is not a Thread, it is a Runnable. Call it something else.

Simplifications.

There are some tricks you can play that will simplify your code a bit.

Firstly, the ThreadTest class (which is a Runnable), does not need to exist at all. It is just a very light-weight container.

Consider the following multi-threaded loop method:

public void runTestMultithreading(final Configuration config, int numThread){
 ExecutorService es = Executors.newCachedThreadPool();
 List<List<Report>> splitted = splitReports(reportsToBeTested, numThread);
 for (final List<Report> reportsOfThread : splitted) {
 es.execute(new Runnable(){
 public void run() {
 runTests(reportsOfThread, config);
 }
 });
 }
 es.shutdown();
 es.awaitTermination(8, TimeUnit.HOURS);
}

Note how we use an anonymous class in here, and we can do it by making the config parameter final, as well as the relatively unknown final inside the for-each loop for (final List<...> ....)

• robermann: Of course this answer is assuming that the multithreading as implemented by OP is correct, isn't it?
• holap: Thank you, the first is a cut&paste error. The correct code is for(Report report : reports), where reports is the parameter of the function (edited the answer). The code is simplified, actual error handling is different. Good point about names (but company convention apply too) and anonymous class. I will pay attention to your suggestions next time. Anyway, apart style improvement and simplification (always useful), is there anything wrong that could make the program fail?
• rolfl: @robermann I can see only one bug.... including concurrency issues.... (I have assumed that the called methods that are not documented in the question, are thread-safe).
• rolfl: @holap - there is nothing in the code you posted here that would indicate a multi-threading problem. unholysampler pointerd out the method calls which you have not posted here may in fact have problems.
• holap: Added the other methods. Hope it make more clear.

Answer 2

1. ExecutorService.awaitTermination has a return value:

   true if this executor terminated and false if the timeout elapsed before termination

   You should check that and at least print a warning to log if it's false.

2. Consider setting an UncaughtExceptionHandler for the executor (through a ThreadFactory). (ThreadFactoryBuilder from Guava has a great API for that.)

3. It's a little bit surprising that field declarations call probably complicated methods:

   final JasperRestfulClient restClient = new JasperRestfulClient();
   final List<Report> reportsToBeTested = restClient.getReports();
   

   I'd put them into a constructor.

4. These fields could be private:

   final String format = "xml";
   final String directoryReport = "\\var\\reports";
   final JasperRestfulClient restClient = new JasperRestfulClient();
   final List<Report> reportsToBeTested = restClient.getReports();
   

   (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

5. fileName is rather one word, I'd not capitalize the n.

• Kirby: Curious, why should complicated methods be moved to a constructor? If they're in the field initialization, they're still invoked in the same sequence as a constructor.
• palacsint: @Kirby: I think (so it's more or less subjective) they're easier to follow and debug, especially when the called method throws an exception and you have to analyze the stacktrace. JasperRestfulClient seems to be an object which uses network calls, so I guess its getReports() method could throw exceptions. Thanks for the good question! I guess I'll update the answer too.

Answer 3

The contents of getFilePath(), compareWithReference() and restClient.runReport() were not posted, so we can't say if the code is or isn't thread safe. The call to runRport() would be my first thought of a problem since the same instance is used in each thread.

• holap: You are right, I wanted to keep the code short in the question. That functions use only final members or local variables, shouldn't have side effects. I will add them tomorrow when at work.
• unholysampler: @holap: final ensures the reference doesn't change. However, if the reference is to a mutable object, more needs to be done to make it thread safe.
• holap: Unfortunately didn't find immutable collection in standard java api. One side effect is actually present (edited the question), but it should be thread-safe. Thank you.
• holap: Added missing methods.

Answer 4

You don't need to split the reports - you should use a fixed thread pool to control your 'split'

 public void runTestMultithreading(final Configuration config, int numThread){
 ExecutorService es = Executors.newFixedThreadPool(numThread);
 for (final Report report : restClient.getReports()) {
 es.execute(new Runnable() {
 public void run() {
 String fileName = getFilePath(directoryReport, report, config);
 restClient.runReport(report.getPath(), format,config, fileName);
 compareWithReference(fileName, report, config);
 }
 );
 }
 es.shutdown();
 es.awaitTermination(8, TimeUnit.HOURS);
 }

if you need the result of each thread you should futures - and gather the result at the end.

Answer 5

Notes for the edit:

1. You really should replace the complex filesAreIdentical with FileUtils.contentEquals (from Apache Commons IO).

   It probably well-tested and it contains some further optimizations:

   This method checks to see if the two files are different lengths or if they point to the same file, before resorting to byte-by-byte comparison of the contents.

2. As far as I see you are using Apache HTTP Client 4.x which looks thread-safe but make sure that the actual httpclient instance you are using is really thread-safe.

3. instream is unused here, you could remove it:

   public void runReport(String url, String destinazione) throws IOException {
    HttpGet httpget = null;
    InputStream instream = null;
    try {
    File file = new File(destinazione);
    file.getParentFile().mkdirs();
    file.createNewFile();
    httpget = new HttpGet(server + url);
    HttpResponse response = httpclient.execute(httpget);
    writeResponceToFile(response, file);
    }
    finally {
    if (httpget != null) {
    httpget.abort();
    }
    }
   }
   

4. You could also restructure the loop and be able to remove the null check:

   File file = new File(destinazione);
   file.getParentFile().mkdirs();
   file.createNewFile();
   final HttpGet httpget = new HttpGet(server + url);
   try {
    HttpResponse response = httpclient.execute(httpget);
    writeResponceToFile(response, file);
   } finally {
    httpget.abort();
   }
   

   If new HttpGet() throws an exception the reference will be null, so can't call abort anyway.

5. In writeResponceToFile you could do the same (I suppose getContent() never returns null but check this, I'm not sure about that), and using a guard clause would make the code flatten:

   private void writeResponceToFile(HttpResponse response, File file) throws IOException {
    HttpEntity entity = response.getEntity();
    if (entity == null) {
    return;
    }
    final InputStream instream = entity.getContent();
    try {
    BufferedInputStream bis = new BufferedInputStream(instream);
    BufferedOutputStream bos = new BufferedOutputStream(new FileOutputStream(file));
    int inByte;
    while ((inByte = bis.read()) != -1) {
    bos.write(inByte);
    }
    bis.close();
    bos.close();
    } finally {
    if (instream != null) {
    instream.close();
    }
    }
   }
   

6. The loop above could be slow since it copies the content in one byte chunks. There are better alternatives:

   • In Java 7: Files.copy(InputStream in, Path target, CopyOption... options)
   • In Apache Commons IO FileUtils.copyInputStreamToFile(InputStream source, File destination)

   copyInputStreamToFile uses a more efficient 4 kbyte buffer and I guess it contains other optimizations, solutions to corner cases etc.

   (See also: Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

7. You should close the output stream in a finally block or use try-with-resources. See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

8. I agree with @user39078 that you don't need the splitReports. Anyway, it's good to know that there is an existing method for that: Guava's Lists.partition

   List<List<T>> Lists.partition(List<T> list, int size)
   

9. createSublist also could be replaced with List.subList.

10. This flag should be properly synchronized (or probably could be volatile):

    errors = true;
    

    (Or you could use an AtomicBoolean.)

• holap: Thank you very much. You gave me some very interesting tips. Well.. shame on me I forgot the volatile keyword. Sorry, as I have to translate the code from my mother language to english I can't just cut and paste the original code. According to www.ibm.com/developerworks/java/library/j-jtp06197 volatile should suffice.