1

I have an activity with 3 buttons: create, start and cancel. Button create creates a new thread, button start runs it and button cancel stops this thread. And my problem is that thread isn't interrupted after calling interrupt method (this action is performed after clicking on cancel button). I know, that in my thread I should check if thread is interrupted or no. I added it, but interrupting still doesn't work. Here's my code:

private View.OnClickListener listener = new View.OnClickListener() {
        @SuppressLint("HandlerLeak")
        @Override
        public void onClick(View v) {
            switch (v.getId()){
                case R.id.create_button:
                    thread = new Thread(new Runnable() {
                        @Override
                        public void run() {
                            int i;
                            for (i = 0; i<10;i++){
                                final int finalI = i;
                                new Handler().postDelayed(new Runnable() {
                                    @Override
                                    public void run() {
                                        textCounter.post(new Runnable() {
                                            @Override
                                            public void run() {
                                                textCounter.setText(String.valueOf(finalI));
                                            }
                                        });
                                    }
                                }, 500*i);
                                if (thread.isInterrupted()){
                                    Log.i(getClass().getSimpleName(), "Thread is interrupted");
                                    return;
                                }
                            }
                            new Handler().postDelayed(new Runnable() {
                                @SuppressLint("SetTextI18n")
                                @Override
                                public void run() {
                                    textCounter.setText("Done!");
                                }
                            },(i+1)*500);
                        }
                    });

                    break;
                case R.id.start_button:
                    thread.run();
                    break;
                case R.id.cancel_button:
                    thread.interrupt();
                    Log.i(getClass().getSimpleName(), "Cancel Button clicked");
                    break;
            }
        }
    };

So, why thread isn't interrupted and how can I solve this problem?

Sergei Mikhailovskii
  • 2,100
  • 2
  • 21
  • 43
  • 2
    @XavierRubioJansana correctly identified your problem - as an alternative to his solution you can also cancel any outstanding delayed handlers by calling their `removeCallbacks`. To do this you'll need a reference to each handler in the cancel case. see https://stackoverflow.com/a/4378606/2711811 and follow any links in that answer. –  Mar 19 '19 at 16:57

2 Answers2

2

Your thread is quickly adding 11 tasks (10 + the last one) to this handler you're creating and then dying. These tasks have a delay, and then the message queue will take care of running the 10 + 1 runnables. To do what you're trying to do you should make the thread to wait 500ms between each loop.

Something similar to this:

                thread = new Thread(new Runnable() {
                    @Override
                    public void run() {
                        int i;
                        for (i = 0; i<10;i++){
                            final int finalI = i;
                            textCounter.post(new Runnable() {
                                @Override
                                public void run() {
                                    textCounter.setText(String.valueOf(finalI));
                                }
                            });
                            try {
                                Thread.sleep(500);
                            } catch (InterruptedException ignored) {
                                Log.i(getClass().getSimpleName(), "Thread is interrupted");
                                return;
                            }
                            if (thread.isInterrupted()){
                                Log.i(getClass().getSimpleName(), "Thread is interrupted");
                                return;
                            }
                        }
                        textCounter.post(new Runnable() {
                            @Override
                            public void run() {
                                textCounter.setText("Done!");
                            }
                        });
                    }
                });
Xavier Rubio Jansana
  • 6,388
  • 1
  • 27
  • 50
  • Why the downvote? Even if this code is ugly, basically is doing what the author of the question is asking. Of course, there are much better ways of doing it. – Xavier Rubio Jansana Mar 19 '19 at 15:15
  • 1
    This is the right answer, I would just add that `isInterrupted()` returns `false` because the thread was `interrupt()-ed` long after it had already terminated, and the design behavior of `isInterrupted()`, in this case, is to return `false`. – greeble31 Mar 19 '19 at 23:12
2

There are many problems with your code

Problem 1: When users click on Create button, you create a new thread, it is not expected behavior as you want, so just create a new thread if it is not created yet or terminated.

Problem 2: When users click on Start button

thread.run();

This line does not start the thread, it just executes the code in run method on calling thread, in this case main/UI thread. To start a thread you must use start method. Make sure you start the thread if it is has been created.

Problem 3: When users click on Cancel button

thread.interrupt();

Because there is no thread started so this line will do nothing.

Problem 4: From your description, you want to use a thread to increase counter on a TextView from 0 to 9 each 0.5 second, then display "Done". Your code is incorrect and contains many redundant.

Solution: You can following this code.

private View.OnClickListener listener = new View.OnClickListener() {
    @Override
    public void onClick(View view) {
        switch (view.getId()) {
            case R.id.create_button:
                // Only create a new thread if it is not created or it is terminated.
                if (thread == null || thread.getState() == Thread.State.TERMINATED) {
                    thread = new Thread(new Runnable() {
                        @Override
                        public void run() {
                            try {
                                for (int i = 0; i < 10; i++) {
                                    final int finalI = i;

                                    // This will post a message to main/UI thread.
                                    textCounter.post(new Runnable() {
                                        @Override
                                        public void run() {
                                            textCounter.setText(String.valueOf(finalI));
                                        }
                                    });

                                    // Sleep current thread in 0.5 second before running next step.
                                    Thread.sleep(500);
                                }

                                // Display Done after finishing counter.
                                textCounter.post(new Runnable() {
                                    @SuppressLint("SetTextI18n")
                                    @Override
                                    public void run() {
                                        textCounter.setText("Done!");
                                    }
                                });
                            } catch (InterruptedException e) {
                                // Display Cancelled if the current thread is cancelled.
                                textCounter.post(new Runnable() {
                                    @SuppressLint("SetTextI18n")
                                    @Override
                                    public void run() {
                                        textCounter.setText("Cancelled!");
                                    }
                                });
                            }
                        }
                    });
                } else {
                    Toast.makeText(MainActivity.this,
                            "Thread is already created. No need to create anymore.",
                            Toast.LENGTH_SHORT)
                            .show();
                }
                break;
            case R.id.start_button:
                // Start thread if it is created.
                if (thread != null && thread.getState() == Thread.State.NEW) {
                    thread.start();
                } else {
                    Toast.makeText(MainActivity.this,
                            "Thread is not created yet or it is running.",
                            Toast.LENGTH_SHORT)
                            .show();
                }
                break;
            case R.id.cancel_button:
                // Cancel the thread if it is running.
                if (thread != null && thread.isAlive()) {
                    thread.interrupt();
                } else {
                    Toast.makeText(MainActivity.this,
                            "Thread is not running yet.",
                            Toast.LENGTH_SHORT)
                            .show();
                }
                break;
        }
    }
};
Son Truong
  • 13,661
  • 5
  • 32
  • 58
  • Didn't notice the problem 2, good catch. I focused on the problems with the delays and finally didn't notice it. BTW @sergei-mikhailovskii this is a super common mistake with threads. Also, a global `try-catch` instead of both checking `isInterrupted()` and catching the exception seems like a good tradeof here to simplify a little bit the code. – Xavier Rubio Jansana Mar 20 '19 at 08:20