Bug in the tests of BackgroundExecutor.cancelAll

[Goutte]

I noticed that BackgroundExecutor.cancelAll did not work for me in the field, with the latest build from develop.

I had had a look at the tests earlier, and saw these two assertions, that I thought were useless. I mean, there is no use of the list in either of the assertions.

So I tried to "fix" (still unsure about what I'm doing) them by replacing NB_ADD by list.size() in the for's exit condition :

for (int i = 0; i < list.size(); i++) {
    Assert.assertTrue("Items must be only from uncancelled tasks", i < NB_ADD);
}

The tests still pass, but I'm still having trouble in my own in-the-field tests, possibly because I'm trying to cancel a task that is already running (a very long http request). I quintuple-checked that I'm using proper ids this time.

I can make a PR for the small fix of the tests, but I'd rather get to the bottom of why it ain't working for me first, by trying to reproduce this in the test-suite.

[Goutte]

FYI, the PR branch is over there. Please don't merge yet !

[Goutte]

Especially as my fix did almost nothing to fix the uselessness of the test.

Now, it's better :

for (int i = 0; i < list.size(); i++) {
    Assert.assertTrue("Items must be only from uncancelled tasks", list.get(i) < NB_ADD);
}

[Goutte]

Okay, I delved deeper into Future.cancel(), and it appears it's not behaving in the way of the least surprise. Ouch. There is no bug in BackgroundExecutor.cancelAll. It's much, much more complicated than that.

Refs:

• http://stackoverflow.com/questions/9536555/utility-of-future-cancelboolean-method
• http://stackoverflow.com/questions/28691081/whether-method-cancel-in-java-util-concurrent-future-shoud-be-blocking

And an interesting reading about safely cancelling tasks in executors : https://10kloc.wordpress.com/2013/12/24/cancelling-tasks-in-executors/

Sometime, it becomes necessary to support non-standard task cancellation – especially when the task relies on blocking or long-running methods that are oblivious to interruption. E.g. when you call ServerSocket.accept(), it starts waiting for a client connection. The catch-22 is that it will ignore all interruption requests and if this function is called in a thread, you cannot stop that thread using interrupts.

---

Should I make a PR for the tests fix ? (note: I'm still struggling with git rebases, so if you want me to rebase or something you'll need to hold my hand or make me RTFM)

[WonderCsabo]

@Goutte sorry for the very late response, but if you are still interested, PRs are always welcome. :)