Future cancel not working with executorService for multiple threads

Question

I am trying write an api to stop multiple threads with java executorService and Future.

Below is my code:

@RestController
@RequestMapping("/work")
public class WorkController {

    final WorkService service;
    private List<Future<?>> future_list;

    public WorkController(WorkService service) {
        this.service = service;
        this.future_list = null;
    }

    @RequestMapping(value="/start",
            method=RequestMethod.POST,
            consumes="application/json")

    public String startWorks(@RequestBody List<Long> workIds) throws IOException {

        // create runnables based on work ids
        int work_number = workIds.size();
        Collection<Runnable> worker_list= new ArrayList<>();
        for (int i = 0; i < work_number; i++) {
            Runnable worker = service.createWorkRunnable(workIds.get(i));
            worker_list.add(worker);
        }

        // 4 thread pool
        ExecutorService executor;
        executor = Executors.newFixedThreadPool(4);

        for (Runnable worker : worker_list) {
            Future<?> future = executor.submit(worker);
            future_list.add(future);
        }

        executor.shutdown();
        while (!executor.isTerminated()) {
        }

        return "Finished all threads";
    }


    @RequestMapping(value="/stop",
            method=RequestMethod.POST)

    public String stopWorks() {
        for (Future<?> f : future_list) {
            Boolean paused = f.cancel(true);
        }
        return "Stoped";
    }

}

When I called stopWorks while runnables are executing, I get

[ERROR] 2018-08-21 06:46:05.275 [http-nio-8090-exec-5] [dispatcherServlet] - Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Re
quest processing failed; nested exception is java.lang.NullPointerException] with root cause
java.lang.NullPointerException: null

It seems that the future is not assigned. Am I on the right track to try interrupt on multiple threads. If so, what did I do wrongly? Thank you

Answer 1

The main issue is that you don't initialize future_list in the constructor. Further, it is better to inject the ExecutorService. I have reworked the code a bit to also use other best practises:

import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;

@RestController
@RequestMapping("/work")
public class WorkController {

    private final WorkService service;
    private final ExecutorService executor;
    private List<Future<?>> futureList;

    public WorkController(WorkService service, ExecutorService executor) {
        this.service = service;
        this.executor = executor;
        this.futureList = new ArrayList<>();
    }

    @PostMapping(value = "/start", consumes = "application/json")
    public String startWorks(@RequestBody List<Long> workIds) {
        // create runnables based on work ids
        Collection<Runnable> workerList = new ArrayList<>();
        for (Long workId : workIds) {
            Runnable worker = service.createWorkRunnable(workId);
            workerList.add(worker);
        }

        for (Runnable worker : workerList) {
            Future<?> future = executor.submit(worker);
            futureList.add(future);
        }

        return "Finished all threads";
    }

    @PostMapping("/stop")
    public String stopWorks() {
        for (Future<?> f : futureList) {
            f.cancel(true);
        }
        return "Stopped";
    }
}

To have an ExecutorService for injection, create a bean like this:

@Bean
public ExecutorService executorService() {
    return Executors.newFixedThreadPool(4);
}

Note that are still issues even with this code:

• The futureList is never cleaned up, so it will grow without bounds. Handling this correctly is not as easy as it seems as there are multiple threads involved (startWorks can be called multiple times, also stopWorks)
• Should it be allowed to call startWorks multiple times if there is already work busy?

Answer 2

Clearly you did not initiate future_list:

public WorkController(WorkService service) {
    this.service = service;
    this.future_list = null; // assign it a List instead null
}