I am doing these exercise:
Write a program that walks a directory tree and generates a thread for each file. In the threads, count the number of words in the files and, without using locks, update a shared counter that is declared as public static long count = 0; Run the program multiple times. What happens? Why?
Fix the program of the preceding exercise with using a lock.
Fix the program of the preceding exercise with using a LongAdder.
And I wrote the following program, in which
CountWordThread answers exercise 1,CountWordLockThread answers exercise 2, andCountWordLongAdderThread answers exercise 3.Java code follows:
import java.io.*;
import java.util.*;
import java.nio.file.*;
import java.util.concurrent.*;
import java.util.concurrent.locks.*;
import java.util.concurrent.atomic.*;
import java.util.stream.*;
import java.util.regex.*;
public class ThreadedCountWord {
public long count = 0;
LongAdder la = new LongAdder();
public class CountWordThread extends Thread {
private File f;
CountWordThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
count ++;
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
ReentrantLock lock = new ReentrantLock();
public class CountWordLockThread extends Thread {
private File f;
CountWordLockThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
// It's important to wrap your code into a
// try/finally block to ensure unlocking in case
// of exceptions.
lock.lock();
try {
count++;
} catch (Exception e) {
e.printStackTrace();
} finally {
lock.unlock();
}
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
public class CountWordLongAdderThread extends Thread {
private File f;
CountWordLongAdderThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
la.increment();
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
public void runThreads(Stream<Path> s) {
// 1. this MAY get inconsistent results
try {
count = 0;
ExecutorService executor = Executors.newCachedThreadPool();
s.forEach(p -> {
CountWordThread t = new CountWordThread(p.toFile());
t.start();
executor.submit(t);
});
executor.shutdown();
executor.awaitTermination(60, TimeUnit.SECONDS);
System.out.printf("(NoLock) count: %d\n", count);
} catch (Exception e) {
e.printStackTrace();
}
}
public void runThreadsWithLock(Stream<Path> s) {
// 2. this SHOULD NOT generate in-consistent results
try {
count = 0;
ExecutorService executor = Executors.newCachedThreadPool();
s.forEach(p -> {
CountWordLockThread t = new CountWordLockThread(p.toFile());
t.start();
executor.submit(t);
});
executor.shutdown();
executor.awaitTermination(60, TimeUnit.SECONDS);
System.out.printf("(Lock) count: %d\n", count);
} catch (Exception e) {
e.printStackTrace();
}
}
public void runThreadsWithLongAdder(Stream<Path> s) {
// 3. this SHOULD NOT generate in-consistent results
try {
count = 0;
ExecutorService executor = Executors.newCachedThreadPool();
s.forEach(p -> {
CountWordLongAdderThread t = new CountWordLongAdderThread(p.toFile());
t.start();
executor.submit(t);
});
executor.shutdown();
executor.awaitTermination(60, TimeUnit.SECONDS);
System.out.printf("(LongAdder) count: %d\n", la.sum());
la.reset();
} catch (Exception e) {
e.printStackTrace();
}
}
public static void main(String[] args) {
// run multi times
try {
for (int i = 0; i < 20; i ++) {
Path path = Paths.get(".");
Stream<Path> sp = Files.walk(path);
Stream<Path> s = sp.filter(p -> p.toString().endsWith(".java")
&& Files.isRegularFile(p)
&& Files.isReadable(p));
ThreadedCountWord tcw = new ThreadedCountWord();
// tcw.runThreads(s); // 1. this MAY get inconsistent results
tcw.runThreadsWithLock(s); // 2. this SHOULD NOT get inconsistent results
// tcw.runThreadsWithLongAdder(s); // 3. this SHOULD NOT get inconsistent results
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
Almost every time 2 or 3 is run, I got inconsistent answers. And I can not figure out why.
A sample result will be this:
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35862
(Lock) count: 35815 <-- note this
(Lock) count: 35862
(Lock) count: 35862
for exercise 2, and
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35826 <-- note this
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
(LongAdder) count: 35862
for exercise 3.
Can you help me out?
Under the help of @chrylis, I updated my answers with the following code, which runs as expected: (The reason why the code above get wrong answer is exactly what @Ivan says.
import java.io.*;
import java.util.*;
import java.nio.file.*;
import java.util.concurrent.*;
import java.util.concurrent.locks.*;
import java.util.concurrent.atomic.*;
import java.util.stream.*;
import java.util.regex.*;
public class ThreadedCountWord {
public long count = 0;
LongAdder la = new LongAdder();
public class CountWordThread extends Thread {
private File f;
CountWordThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
count ++;
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
ReentrantLock lock = new ReentrantLock();
public class CountWordLockThread extends Thread {
private File f;
CountWordLockThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
// It's important to wrap your code into a
// try/finally block to ensure unlocking in case
// of exceptions.
lock.lock();
try {
count++;
} catch (Exception e) {
e.printStackTrace();
} finally {
lock.unlock();
}
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
public class CountWordLongAdderThread extends Thread {
private File f;
CountWordLongAdderThread(File f) {
this.f = f;
}
@Override
public void run() {
try {
BufferedReader br = new BufferedReader(new FileReader(f));
String line;
String pattern = "(\\w+)";
Pattern r = Pattern.compile(pattern);
while ((line = br.readLine()) != null) {
Matcher m = r.matcher(line);
while(m.find()) {
la.increment();
}
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
public void runThreads(Stream<Path> s) {
// this MAY get inconsistent results
try {
count = 0;
ArrayList<Thread> ts = new ArrayList<>();
s.forEach(p -> {
CountWordThread t = new CountWordThread(p.toFile());
t.start();
ts.add(t);
});
ts.stream().forEach(t -> {
try {
t.join();
} catch (Exception e) {
e.printStackTrace();
}
});
System.out.printf("(NoLock) count: %d\n", count);
} catch (Exception e) {
e.printStackTrace();
}
}
public void runThreadsWithLock(Stream<Path> s) {
// this SHOULD NOT generate in-consistent results
try {
count = 0;
ArrayList<Thread> ts = new ArrayList<>();
s.forEach(p -> {
CountWordLockThread t = new CountWordLockThread(p.toFile());
t.start();
ts.add(t);
});
ts.stream().forEach(t -> {
try {
t.join();
} catch (Exception e) {
e.printStackTrace();
}
});
System.out.printf("(Lock) count: %d\n", count);
} catch (Exception e) {
e.printStackTrace();
}
}
public void runThreadsWithLongAdder(Stream<Path> s) {
// this SHOULD NOT generate in-consistent results
try {
count = 0;
ArrayList<Thread> ts = new ArrayList<>();
s.forEach(p -> {
CountWordLongAdderThread t = new CountWordLongAdderThread(p.toFile());
t.start();
ts.add(t);
});
ts.stream().forEach(t -> {
try {
t.join();
} catch (Exception e) {
e.printStackTrace();
}
});
System.out.printf("(LongAdder) count: %d\n", la.sum());
la.reset();
} catch (Exception e) {
e.printStackTrace();
}
}
public static void main(String[] args) {
// run multi times
try {
for (int i = 0; i < 20; i ++) {
Path path = Paths.get(".");
Stream<Path> sp = Files.walk(path);
Stream<Path> s = sp.filter(p -> p.toString().endsWith(".java")
&& Files.isRegularFile(p)
&& Files.isReadable(p));
ThreadedCountWord tcw = new ThreadedCountWord();
// tcw.runThreads(s); // this MAY get inconsistent results
// tcw.runThreadsWithLock(s); // this SHOULD NOT get inconsistent results
tcw.runThreadsWithLongAdder(s); // this SHOULD NOT get inconsistent results
}
} catch (Exception e) {
e.printStackTrace();
}
}
}
You start your tasks twice: first time with t.start() and second time when submit to executor. And because you do not call t.join() after t.start() to wait for tasks to finish you might get inconsistent result just because you print value before all job is done