1

I thought I had a fairly decent understanding of locks and basic multi-threading concepts, but I'm royally messing something up here.

All the program is supposed to do is receive a filename for a text file and the number of threads you want to use from the user, and then count the number of "http" links in that file before returning that number to the user.

For the life of me, however, I can't get my threads to count properly. I've tried making the "count" variable an atomic integer and using the "incrementAndGet()" function, I've tried placing locks in the code where I thought they should be, and I've tried specifying the necessary functions are synchronized, all to no avail.

I've been doing the reading that I can on locks, concurrency, and whatnot, but I'm apparently doing something very wrong. Could someone explain to me where/why I need to be placing locks in my code (or how to properly use atomic integers, if that would be more effective)?

I originally had a lock for when the "count" variable was used (outside of any loops so it actually did something), but I'm receiving all kinds of numbers when I run the program.

My code looks like this:

import java.io.*;
import java.util.*;
import java.util.concurrent.locks.*;
import java.util.concurrent.atomic.AtomicInteger;

public class Count
{
    static String fname;

    static int count = 0;


    public static void main(String[] arg)
    {
        Scanner in = new Scanner(System.in);

        int numthreads;
        if( arg.length > 0 )
        {
            fname = arg[0];
            numthreads = Integer.parseInt(arg[1]);
        }
        else
        {
            System.out.println("File?");
            fname = in.nextLine();

            System.out.println("Num threads?");
            numthreads = in.nextInt();
        }

        RandomAccessFile raf;
        try
        {
            raf = new RandomAccessFile(fname,"r");
            Thread[] T = new Thread[numthreads];
            for(int i=0;i<numthreads;++i)
            {
                T[i] = new Thread(new Task(i,numthreads,raf));
                T[i].start();
            }
            for(Thread t : T)
            {
                try 
        {
                    t.join();
            } 
        catch (InterruptedException e) 
        {
                    System.out.println("Thread join failed!");
                    e.printStackTrace();
                    System.exit(0);
            }
            }
            System.out.println("The total is: "+count);
        }
        catch(IOException e)
        {
            System.out.println("Could not read file");
        }   
    }
}

class Task implements Runnable
{
    int myid;
    int totaltasks;
    RandomAccessFile raf;
    ReentrantLock L = new ReentrantLock();
    public Task(int myid, int totaltasks, RandomAccessFile raf)
    {
        this.myid=myid;
        this.totaltasks=totaltasks;

        this.raf = raf;

    }
    @Override
    public void run()
    {

        try
    {
            long filesize = raf.length(); //length of the entire file
            long sz = filesize / totaltasks; //length of entire file divided by number of threads gives thread size
            long start = myid*sz; // thread's number * size of thread. How we find start of thread in the file


            raf.seek(start);
            if(start != 0 )
            {
                raf.readLine();
            }

            while(raf.getFilePointer() < (start+sz))
            {

                String s = raf.readLine();

                Scanner sc = new Scanner(s);

                while(sc.hasNext())
                {
                    String w = sc.next();

                    if( w.startsWith("http://"))
                    {
                        Count.count++;

                    }

                }

            }

        }
        catch(IOException e)
        {
            System.out.println("IO error!");
        }
    }
}

If there's bigger issues at work here than me needing to use locks in the proper places, please let me know and I'll at least have something to work with, but as far as I'm aware it should just be a matter of performing locks properly and I'm either not understanding how they work correctly or just putting them in all the wrong places.

Stravask
  • 11
  • 1
  • 3
  • Can you please indicate what exactly you expected you code to achieve and what have you got instead? – PM 77-1 Feb 13 '15 at 21:50
  • @PM77-1 The goal of the code is simply to accurately count the number of http links in a text file while using multiple threads. For example, if the user provides the program with a text file with 200 "http://" links in it, the code should return "200" to the user. However, because I'm not using locks correctly, each thread is able to access the "count" variable at their leisure and is causing race conditions I believe, meaning I'm getting back numbers other than "200" because the threads aren't being restricted by locks the way they should be. Is that clear enough? – Stravask Feb 13 '15 at 21:53
  • 1
    I believe you need *atomic* (in nature) implementation of `sc` to ensure that `sc.next()` never gives the same line multiple times. I think this is where racing originates. – PM 77-1 Feb 13 '15 at 22:01
  • @PM77-1 Okay, i'll try that and see what comes up – Stravask Feb 13 '15 at 23:35
  • 1
    One RandomAccessFile instance is shared between all threads, that is not going to work. Pass the file-name to the Task and create a separate RandomAccessFile instance within the Task (and don't forget to close it in a finally-block at the end of the run-method). Also `Count.count++` is not atomic, set `static int count = new AtomicInteger();` and use `incrementAndGet()` as you already mentioned. – vanOekel Feb 14 '15 at 01:00
  • Was the lock you were using ReentrantLock L = new ReentrantLock();? If so wouldn't that mean that each instance of Task would have it's own lock since it's a property of the task? – Faris Feb 14 '15 at 01:05

0 Answers0