Java concurrency - Is this efficient?

Question

System.out.println("Enter the number of what you would like to do");
System.out.println("1 = Manually enter Options");
System.out.println("2 = Use a text file to pick from pre-existing models");
System.out.println("3 = Exit ");


Scanner sc  = new Scanner(System.in);
try {
    runType = sc.nextInt();
    if(runType > 3) {
        throw new badValue(999, "Not the valid input");
    }
} catch (NullPointerException e) {
} catch (badValue e) {
    e.correctBadValue(runType);
} 

switch (runType) {
case 1:
    Thread a = new SelectCarOption();
    a.run();
case 2: 
    Thread a2 = new BuildCarModelOptions();
    a2.run();
case 3:
    System.exit(1); 

}

} }

So basically, I'm trying to run a program where the thread that is running is determined by a variable runType. If runType is one value, a certain thread will run and if it is the other, the other will run. Is my approach the most efficient? Will it turn out to give any errors?

Why are you starting thread1 and then stopping it when you figure out it is not the correct thread? Just start the thread that you need to run. Also, are you sure that you need threads for this? — Marcelo
– Marcelo, Commented Aug 9, 2013 at 18:20
you are btw doing it wrong. .start() starts the thread. Don't call run() or there is no thread and run is exectued in place in whatever thread you are. — zapl
– zapl, Commented Aug 9, 2013 at 18:23
Even if you have a problem of this kind, you should use a Lock, the thread aquiring Lock will run and others will wait. — Sachin Thapa
– Sachin Thapa, Commented Aug 9, 2013 at 18:42
The code appears to take pains to not actually execute concurrently, so the question that arises is do you actually need concurrent code and threads? As written, you could replace your code with direct calls to 2 methods which would be far more clear. — Trevor Freeman
– Trevor Freeman, Commented Aug 9, 2013 at 18:48

yshavit · Accepted Answer · 2013-08-09 18:38:59Z

5

Long story short, no, this is not how you want to do things.

thread1.run() doesn't start a new thread, it just calls the code in run() on the current thread. What you want is thread1.start().
thread1.sleep(5000) will not make thread1 sleep, it will make the main thread sleep. Thread.sleep is a static method that affects the current thread, and the fact that you're using an instance variable to invoke it (rather than the more traditional Thread.sleep(5000)) doesn't change that.
It makes no sense to start thread2 and then immediately join to it. You may as well just invoke its code directly on the main thread. (Which is what you're doing right now, since you're invoking thread2.run() instead of thread2.start().)

I'm not sure what your end goals are, but this sounds like a case for plain old polymorphism. Create a Runnable and assign it to one of two concrete implementations, depending on the input; then just invoke run() on it. Something like:

Runnable selectStrategy = (runType == 2)
    ? new CarModelOptionsIO()
    : new SelectCarOption()

selectStrategy.run()

If you need a result from this action, you could use a Callable<T> (don't let the package name confuse you; there's nothing inherent to concurrency in that interface) or even create your own interface, which lets you give more meaningful names to the methods (call and run are pretty unhelpfully generic).

edited Aug 9, 2013 at 18:38

answered Aug 9, 2013 at 18:28

yshavit

43.7k8 gold badges90 silver badges127 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

yshavit Over a year ago

You're still using Thread.run. That doesn't start a new thread. It's just a plain ol', normal, standard method invocation. Also, you really shouldn't be catching the NPE and ignoring it.

zapl · Accepted Answer · 2013-08-09 18:36:11Z

1

A programmer had a problem. He thought to himself, "I know, I'll solve it with threads!". has Now problems. two he

A)

you can replace

    Thread thread1 = new SelectCarOption();
    thread1.start();
    thread1.join();

by directly executing whatever run does since the thread that starts the thread just waits.

calling thread   | new thread

   start() ---->
                   run()
   join()        <---

does the same thing as

   run()

Now we can simplify your code to:

   if (runType == 2) {
       doCarModelOptionsIO();
   } else {
       doSelectCarOption()
   }

And you have a much more efficient way.

B)

Don't call the run() method on a thread. Every method called directly is executed in place in your current thread. Thread has the start() method that you call which then calls run() from within that new thread.

edited Aug 9, 2013 at 18:36

answered Aug 9, 2013 at 18:30

zapl

64.1k10 gold badges126 silver badges160 bronze badges

1 Comment

user2668836 Over a year ago

thank you i appreciate the clear answer. I decided to do your approach.

William Morrison · Accepted Answer · 2013-08-09 18:28:06Z

Overall, your code is confused. I suggest reading the concurrency tutorials if you haven't already. Review them if you have read them. Maybe do a few yourself, then come back to this problem.

You say:

If runType is one value, a certain thread will run and if it is the other, the other will run.

To do that you need something like this...

    if (runType == 2) {
        Thread thread1 = new SelectCarOption();
        thread1.run();
        try {
            //join blocks until thread 1 terminates. We don't know that it
            //ever will give your code
            thread1.join(); //stops thread1 to run different thread 
        } catch (InterruptedException e1) {
            e1.printStackTrace();
        }
        Thread thread2 = new CarModelOptionsIO();
        thread2.run();
        try {
            //blocks again, until thread 2 is done running.
            thread2.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    } else {

        try {
            thread1.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        //start some other thread here since runType is different?
    }

lcestari · Accepted Answer · 2013-08-09 18:32:55Z

0

There are many mistakes in your class. First you are using the method run() instead of start(). Second, you should start both threads for your sleep() make sense. That a look on the Oracle Java Se tutorial online to see the basics of Java Multithreading model, that will help a lot.

answered Aug 9, 2013 at 18:32

lcestari

1785 bronze badges

Comments

Ankur Shanbhag · Accepted Answer · 2013-08-09 18:33:05Z

There are several mistakes in your code. and to let you know, the code you have written does not spawn a new thread at all. Few thing to note:

Thread.sleep() is a static method. The below code is misleading:

try {
  thread1.sleep(5000); //stops thread1 to run different thread 
} catch (InterruptedException e1) {
  e1.printStackTrace();
}

You have started thread1 in the main thread and then called sleep method using the newly created thread. But this is not gonna help you. It makes the main thread sleep not thread1. To make thread1 sleep you need to call sleep method within the run() of this thread1 class.
Moreover, sleep() is a static method and should not be called using thread instances, it is misleading.

Also stopping a thread does not necessarily mean it will invoke the other thread. Just remember when it comes to threading, very little is guaranteed.

One more thing :

thread1.run(); // This is incorrect

use thread1.start()

Directly calling run() method does not start a new thread. You need to call start() method to start a new thread. Calling run method directly will execute the contents of the run method in the same thread(from where it is invoked)

Collectives™ on Stack Overflow

Java concurrency - Is this efficient?

5 Answers 5

1 Comment

1 Comment

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

1 Comment

1 Comment

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related