9

Edit:

To test this problem outside of Android environment I've created a Java application that creates an ExecutorService, provides a task of AttackScript (identical class) and then terminates.

This works 100% as expected, the thread is interrupted and the task is stopped.

You don't even have to cancel a task by its Future.cancel(true). ExecutorService.shutdownNow() does the job. Is there something in the Android's Service that somehow messes with the thread pool?

Code that works as expcepted:

public static void main(String[] args) {
        AttackScript script = new AttackScript("http://ninjaflex.com/");

        ExecutorService executor = Executors.newFixedThreadPool(5);
        executor.submit(script);
        executor.submit(script);
        executor.submit(script);
        executor.submit(script);

        sleep(1300);

        // Automatically interrupts threads in the pool.
        executor.shutdownNow();
    }

    private static void sleep(long timeMilli){
        try {
            Thread.sleep(timeMilli);
        } catch(Exception e) {
            System.out.println("Error sleep()");
        }
    }

Original post:

I have an Android Service where it includes an ExecutorService field, responsible to run some tasks.

The tasks are objects of the AttackScript class. I cache the Future references in a Map<String,Future>, called tasks, so that I will be able to cancel them later.

Future future = executor.submit(new AttackScript(attack.getWebsite()));
tasks.put(attack.getPushId(), future);

In Service'sonDestroy() (called when user presses a notification button) I am cancelling all tasks

private void cancelAllTasks() {
    for (Map.Entry<String, Future> futureEntry : tasks.entrySet()) {
        futureEntry.getValue().cancel(true);
    }
}

and then shutdown the executor:

private void shutdownThreadPool() {
     // https://www.baeldung.com/java-executor-service-tutorial
     executor.shutdown();
     try {
         if (executor.awaitTermination(800, TimeUnit.MILLISECONDS))
                executor.shutdownNow();
     } catch (InterruptedException e) {
            executor.shutdownNow();
     }
}

Finally here is the AttackScript class:

public class AttackScript implements Runnable {
    private static final String TAG = "AttackScript";
    private URL url;

    public AttackScript(String website) {
        initializeUrl(website);
    }

    private void initializeUrl(String website) {
        try {
            url = new URL(website);
        } catch (MalformedURLException e) {
            Log.e(TAG, "Wrong url?", e);
        }
    }

    @Override
    public void run() {
        while (!Thread.currentThread().isInterrupted()) {
            readUrl();
        }
        Log.d(TAG, "Stopped requesting from " + url + " server.");
    }

    private void readUrl() {
        InputStream in = null;
        try {
            in = url.openStream();
        } catch (IOException e) {
            Log.e(TAG, "openStream() error.", e);
        } finally {
            closeInputStream(in);
        }
    }

    private void closeInputStream(InputStream in) {
        try {
            in.close();
            Log.d(TAG, "InputStream closed for " + url);
        } catch (IOException e) {
            Log.e(TAG, "Error while closing the input stream.", e);
        }
    }
}

The weird part is that rarely, like 1 out of 10, tasks are interrupted and AttackScript's execution stops. But the other 9 the tasks were not interrupted, continuing to openStreams() on URLs.

Themelis
  • 4,048
  • 2
  • 21
  • 45
  • try this ... (taken from here: https://stackoverflow.com/questions/1561364/the-cause-of-interruptedexception) catch (IOException exception) { // If interrupted this isn't a real I/O error. if (Thread.interrupted()) { throw new InterruptedException(); } else { throw exception; } } – Wayne Jan 29 '19 at 20:13
  • Tried it, without success. I am not reading from the stream, just obtaining it through`url.openStream()` but the latter does throw an `IOException`. Unfortunately on runtime no IOException is thrown, because everything went normal, which means the task is still not interrupted. – Themelis Jan 29 '19 at 20:32
  • Could it be that the Thread has already finished, already performed the closing of the streams? – Wayne Jan 29 '19 at 20:36
  • Nope, unfortunately it's continuing to execute. I am seeing the logs and the network calls on the profiler. – Themelis Jan 29 '19 at 20:43
  • Ok found and article that explains this... Use this... Map> cancellableFutures = new HashMap<>(); Future> future = executor.submit(runnable); cancellableFutures.put(runnable, future); //now you want to abruptly cancel a particular task runnable.cancel(); cancellableFutures.get(runnable).cancel(true); Read more here: https://dzone.com/articles/interrupting-executor-tasks – Wayne Jan 29 '19 at 21:25
  • Thanks, I'll read it right now. – Themelis Jan 29 '19 at 21:28
  • Wow I am so tired I am unable even to read. I will read it tomorrow, it looks interesting, thanks! – Themelis Jan 29 '19 at 21:33
  • When you loop over the futures to cancel them one by one, might it be that one of the cancel operations is failing due to an exception being thrown (because in the JavaDoc of Future, it is stated that "This attempt will fail if the task has already completed, has already been cancelled, or could not be cancelled for some other reason.")? The exception is thrown up from your `onDestroy()` method (and probably not logged somewhere), so `shutdownThreadPool()` never gets called? – Utku Özdemir Jan 29 '19 at 23:22
  • In the app I logged the ‘Future.cancel(true)’ return value and it was always true. Which I guess meant that the attempt did not fail. I also tried to directly ‘shutdownNow()’ but with no effect. The fact that exactly the same code as a Java application runs normally drives me totally crazy! @UtkuÖzdemir – Themelis Jan 29 '19 at 23:31
  • I see. Then I recommend the following steps, one by one, to narrow down the issue: 1- instead of using/checking thread interruption flag, introduce a transient Boolean (or atomicboolean) variable, and implement logic against it. 2- replace url.openstream by a simple println call, to eliminate the possibility of a different behavior caused by the network stack. 3- Change the thread pool implementation/choose the concrete implementation manually. So, in short, try different variants to narrow it down - That’s how I would approach the problem. – Utku Özdemir Jan 30 '19 at 01:01
  • On part 3 you mean to use Threads directly? @UtkuÖzdemir – Themelis Jan 30 '19 at 09:10
  • @Skemelio nope, I mean trying different ExecutorService implementations, such as ThreadPoolExecutor etc. – Utku Özdemir Jan 30 '19 at 11:08
  • Will do, but when you create an `ExecutorService` from `Executors.newFixedThreadPool()` that already creates a `ThreadPoolExecutor` with equal `corePoolSize` and `maximumPoolSize` parameter values and a zero `keepAliveTime` (https://www.baeldung.com/thread-pool-java-and-guava). Got nothing to lose though, so I am trying it @UtkuÖzdemir – Themelis Jan 31 '19 at 11:35
  • 1
    @Skemelio It was only an example, what I meant was, you need to change the parameters of every part of your code, one by one, to narrow the problem down. And the type of ExecutorService is a parameter. If it is already ThreadPoolExecutor, try some other implementation - it can even be a 3rd party implementation. So take my recommendation only as an approach to the problem - because I don’t see anything obviously wrong in your code, so that’s the only thing I could suggest. – Utku Özdemir Jan 31 '19 at 11:39
  • Thanks @UtkuÖzdemir, it was a good suggestion. Just to inform you the `ThreadPoolExecutor`didn't worked so I am going to handle `Thread`s myself, without a thread pool. Maybe something in android is messing with the interruption signal and I cannot see it. What can you do... – Themelis Jan 31 '19 at 11:47
  • 1
    @Skemelio Yes, I suspect that as well. Maybe it is about the thread management of that specific Android version/SDK and/or the vendor flavor... Maybe it's worth trying on different phones and Android versions imo. – Utku Özdemir Jan 31 '19 at 22:17
  • Out of desperation I replaced the thread pool with single `Thread`'s stored in a `Map`. Again the thread's were not interrupted so I completely removed interruption mechanism. Now it's a boolean that controls the execution exposed to others and the threads they stop at least. @UtkuÖzdemir – Themelis Feb 01 '19 at 08:46

2 Answers2

4

Forced to find an alternative solution I completely removed the use of a thread pool and now implementing single Threads, stored in a Map.

The interruption was, again, never happening so an AtomicBoolean is now controlling the Thread's execution.

private AtomicBoolean stopped = new AtomicBoolean(false);

 @Override
    public void run() {
        while (!stopped.get()) {
            readUrl();
        }
}

public void stopExecution() {
        stopped.set(true);
}

It's a desperate move, but the only one that works so far.

Themelis
  • 4,048
  • 2
  • 21
  • 45
2

You answered already with a valid workaround to avoid this issue but I will explain the cause. The fault is not with the ExecutorService but with the thread's interrupt status being cleared silently by the network library.

As you and another commenter have found this could very well depend on the particular device you are using and its Android version.

As of Android 4.4 OkHttp is used as the HttpUrlConnection. There is a race condition between when each thread is interrupted and whether or not the InputStream has been closed in older versions.

As a part of the close() call this code is eventually executed:

public void throwIfReached() throws IOException {
    if (Thread.interrupted()) {
        throw new InterruptedIOException("thread interrupted");
    }

    if (hasDeadline && deadlineNanoTime - System.nanoTime() <= 0) {
        throw new InterruptedIOException("deadline reached");
    }
}

You can see based on the Thread.interrupted() call it clears the interrupt status of the thread and never sets it again.

To make matters worse it looks like you can instead rely on the InterruptedIOException but that is silently handled internally when closing the stream so you have no chance to handle it.

Your code sample worked for me when using a more recent version of OkHttp. In later versions it looks like more care was taken to keep the interrupt status and it actually works as expected.

However, based on some searching it looks like historically interrupts do not play nicely with OkHttp and to stop a request they recommend Call.cancel() where possible instead.

George Mulligan
  • 11,813
  • 6
  • 37
  • 50
  • Never imagined someone would answer after so many days, that's why I had to implement threads by hand. I know that something is messing with the interruption status, but I haven't used `OkHttp`. I just obtained an `InputStream`from a `URL`. – Themelis Feb 05 '19 at 22:33
  • Yeah OkHttp is used with `url.openStream()` since Android 4.4. You should be able to validate by debugging and stepping several times into that method. You likely will not see source but you will see OkHttp related classes in the stack frames deeper you go. You can also manually verify what I mention here by interrupting the thread just before `in.close()`. After the `close()` call the thread will no longer be interrupted if you are hitting the same scenario. – George Mulligan Feb 05 '19 at 22:41
  • If I got it right you state that `OkHttp` is used when `java.net.URL.openStream()` is called (since Android 4.4)? If yes, how is that possible that a class from the `java.net` package uses an external library? – Themelis Feb 05 '19 at 23:24
  • 1
    You can look at the Android source and see. You need to remember that the Android API is different than the Java API. AOSP has changed source code in places. `java.net.URL` is an example. Either open the source directly in Android Studio or take a look [here](https://android.googlesource.com/platform/libcore/+/refs/heads/master/ojluni/src/main/java/java/net/URL.java) as an example. Search for okhttp and you will see it uses reflection. – George Mulligan Feb 06 '19 at 03:28
  • Wow... `else if (protocol.equals("http")) { handler = (URLStreamHandler)Class. forName("com.android.okhttp.HttpHandler").newInstance();` – Themelis Feb 06 '19 at 08:10
  • 1
    Don't know what to say... Is this stuff like that which lead to the trial of Google and Oracle? – Themelis Feb 06 '19 at 08:12
  • Yes. I am not too familiar with the trial and do not want to say anything misleading but basically Oracle believes what Google did with java for Android goes beyond fair use. – George Mulligan Feb 06 '19 at 16:02
  • Ok the trial is not my business but it's shocking when you think you were using a class from a java framework, like `java.net.URL`, and finding out that this was not actually quit true. – Themelis Feb 06 '19 at 16:32
  • That's why out of Android, same code worked as expected! – Themelis Feb 06 '19 at 16:33