0

I'm trying to simulate a printer setup in which monochrome and colour prints are done sequentially. When removing a print statement, my code produces unexpected results.

The function in question is request(). I have researched this and came across lots of articles where it is discussed that this revolves around thread synchronization and adding the synchronization keyword to the function, or to make the variable in question volatile.

I believe that my three threads are simultaneously popping the list and retrieving the same data as the output WITHOUT printouts is:

(0) M3 uses head 1 (time: 3)
(0) M3 uses head 2 (time: 3)
(0) M3 uses head 3 (time: 3)

Where as including the printouts is:

grabbed job M1
grabbed job M2
(0) M1 uses head 1 (time: 4)
grabbed job M3
(0) M2 uses head 2 (time: 5)
(0) M3 uses head 3 (time: 3)

However, I have from the beginning of this project had the synchronized keyword in my function. I also have tried replacing the keyword with a binary semaphore within the function. This article was found to be informative, but not an answer to my exact scenario: Questions about thread+loop not working without print statement

My code:

import java.util.LinkedList;

public class Printer{

    public static int TIME = 0;
    private static boolean COLOURMODE = true;
    private static LinkedList<Job> jobs;
    private static Job tempJob;
    private PrinterHead printHead1;
    private PrinterHead printHead2;
    private PrinterHead printHead3;


    public Printer(LinkedList<Job> jobs) {
        this.jobs = jobs;
        printHead1 = new PrinterHead(this,1);
        printHead2 = new PrinterHead(this,2);
        printHead3 = new PrinterHead(this,3);
        printHead1.start();
        printHead2.start();
        printHead3.start();
    }

    public static boolean hasJobs(){
        if(jobs.isEmpty()){
            return false;
        }
        else{
            return true;
        }
    }

    public synchronized Job request(){
        tempJob = jobs.peek();

        if(COLOURMODE){
            if(tempJob.type == 'C'){
                //System.out.println("grabbed job " + tempJob.jID);
                return jobs.pop();
            }
            else{
                COLOURMODE = false;
                printHead1.TIME = TIME;
                printHead2.TIME = TIME;
                printHead3.TIME = TIME;
            }
        }
        else if(!COLOURMODE){
            if(tempJob.type == 'M'){
                //System.out.println("grabbed job " + tempJob.jID);
                return jobs.pop();
            }
            else{
                COLOURMODE = true;
                printHead1.TIME = TIME;
                printHead2.TIME = TIME;
                printHead3.TIME = TIME;
            }
        }
        return null;
    }
}

edit: PrinterHead class added.

import java.util.concurrent.Semaphore;

public class PrinterHead extends Thread {

    private static Job job;
    public static int TIME = 0;
    private int headNum;
    private static Printer printer;
    private static Semaphore mutex = new Semaphore(1, true);

    public PrinterHead(Printer printer, int headNum){
        this.headNum = headNum;
        this.printer = printer;
    }

    public void run(){
        while(printer.hasJobs()){
            job = printer.request();

            if (job != null){
                print();
                if(printer.TIME < job.pages+TIME){
                    printer.TIME = job.pages+TIME;
                }
                try{
                    this.sleep(1000*job.pages);
                }
                catch(Exception e){
                    System.out.println("some shit broke in printhead " + headNum);
                    return;
                }
            }
        }
    }

    private void print(){
        try{
            mutex.acquire();
            System.out.println("("+TIME+") "+ job.jID+" uses head "+headNum + " (time: "+job.pages+")");
            mutex.release();
        }
        catch(Exception e){
            System.out.println("some shit broke in printout " + headNum);
            return;
        }
    }
}

My Main creates one Printer instance and a LinkedList of Jobs.

If you need more, ask me please and thank you.

jeffbobmate
  • 39
  • 1
  • 1
  • 6
  • Can you post the code for `PrinterHead` ? – IllegalArgumentException Oct 03 '17 at 12:24
  • I have just posted the code. – jeffbobmate Oct 03 '17 at 12:47
  • 1
    Even without searching for concurrency issues, I see quite some design issues, such as extending `Thread` instead of implementing `Runnable`, sharing of instance variables such as `COLOURMODE`, `tempJob`, `printer`, lack of encapsulation. – M. le Rutte Oct 03 '17 at 12:52
  • I am aware of such design issues and apologize. Extending Thread is how I was taught/told to do this, you're right about COLOURMODE and tempJob, and I planned to add encapsulation once I had this issue resolved as there is still much more to be done afterwards. – jeffbobmate Oct 03 '17 at 12:57
  • 1
    First clean up your code. By using things `private static Job tempJob;` you are creating multi-threading issues that can be hard to solve. Clean code first, then the next step. Maybe https://codereview.stackexchange.com/ is a better place to get a code review. – M. le Rutte Oct 03 '17 at 13:07
  • Ok thank you for the assistance! It is clear I need to review what I know about the `static` keyword etc. – jeffbobmate Oct 03 '17 at 13:14
  • Whoever told you to extend Thread is just flat out wrong. It works but it's completely the wrong way to do this. – Tim B Oct 03 '17 at 13:14
  • https://stackoverflow.com/questions/16489026/what-is-the-main-advantage-of-extending-thread-class-or-when-to-extend-thread-i and https://stackoverflow.com/questions/541487/implements-runnable-vs-extends-thread are worth reading – Tim B Oct 03 '17 at 13:16
  • I will have a read. Thank you. – jeffbobmate Oct 03 '17 at 13:17

0 Answers0