Cancel background tasks #569

rom1v · 2013-04-22T15:33:32Z

Like someone asked on the mailing list, I think it would be useful to be able to cancel background tasks, typically in onStop():

    @Override
    protected void onStop() {
        super.onStop();
        boolean mayInterruptIfRunning = true;
        BackgroundExecutor.cancelAll("longtask", mayInterruptIfRunning);
    }

    @Background(id="longtask")
    public void doSomethingLong() {
        // ...
    }

I implemented it over my changes adding the background execution serialization feature (I changed a bit the implementation).

@Background now supports id (used for cancellation), delay (was already implemented) and serial (for serializing tasks) attributes, and any association of them.

As the background execution feature must be reliable, and the interactions between serializing and cancelling must behave correctly, I wrote a set of functional tests.

A ScheduledExecutorService *is* an Executor, so we can use the same to avoids to create too many threads. The execute(runnable) method becomes a particular case of execute(runnable, delay), with delay=0. As the user can change the executor, delayed executions are not supported if the given executor does not support scheduling. This unification paves the way for adding serial execution feature.

@background

Adds an optional serial attribute to @background: @background(serial="some_id") This idea comes from androidannotations#309: androidannotations#309 The principle of this implementation is to keep a separate queue for each serial identifier, and to give each task to the (unique) executor only after the previous task with the same serial identifier (if any) has completed execution. It guarantees that all tasks with the same serial identifier will be called in the order, one at a time (but not necessarily on the same thread). It is still compatible with the delay attribute. For example: @background(serial="some_id", delay=2000) executes the task in background after at least 2 seconds *and* after the previous task requested with serial="some_id" (if any) have completed execution.

Test if actions are sequential as expected.

Avoid try/catch InterruptedException of Thread.sleep().

The previous implementation used a set for serial running and a map of lists of tasks for managing the serial queues. In practice, there will be very few parallel tasks, using maps and creating/destroying lists is complex and unefficient. A better approach is to use only a list of tasks (only the ones we need to keep, having a non-null serial) and run through it sequentially for retrieving tasks. Moreover, it is more general, and paves the way for adding a task cancellation feature.

Extract the surround-with-try-catch part of the anonymous Runnable generation, in order to reuse it for generating other anonymous classes.

Instead of create a new Runnable instance which will be wrapped by a Task instance, directly create a Task (which is a Runnable).

@background

Adds an optional id attribute to @background: @background(id="some_id") The user is able to cancel all background tasks having a specified id: BackgroundExecutor.cancelAll("some_id");

Tests the serialization and cancellation of background tasks.

If the executor is replaced by another not supporting cancellation (not extending ExecutorService), it failed the first time a task with a non-null id was requested. Now, it just warns during cancellation.

melnikovdv · 2013-04-26T11:32:23Z

Very cool, thx

rom1v · 2013-04-26T11:38:36Z

@melnikovdv I didn't see the issue #478 (because it was closed). I reference it here.

rom1v · 2013-05-19T18:27:52Z

this comment answers to a deleted comment by gebing

@gebing Which javac version do you use?

There is such a bug in Oracle JDK version which is fixed in 6u32.

gebing · 2013-05-19T18:46:05Z

sorry, it is my fault to compile with duplicate jars

DayS · 2013-05-21T20:53:07Z

I was looking at your PR and I saw all the comments in the code and commit descriptions. I was just... wow. Thanks for that 👍

However, we have to do lots of tests on this feature

rom1v · 2013-05-22T11:22:52Z

@DayS

I made this stupid code to test your feature:
…
But I only had one log... It seems that the computed delay is always near of Long.MAX_VALUE because it wasn't initiated

This bug was fixed in commit 5254f56. If you test cancel branch, it should work.

I could fix it by changing targetTime from long to Long and replacing line 203 by this:

/* compute the remaining delay */
int delay;
if (targetTime == null) {
   delay = 0;
} else {
   delay = Math.max(0, (int) (nextTask.targetTime - System.currentTimeMillis()));
}

I solved it a similar way, I just used 0 as special value instead of using a Long:

0 is in the past (epoch) ;
even if date device is configured to be just before epoch (very unlikely), the probability to be exactly at time 0 is very low ;
it avoids to instantiate a Long and dereference it.

Another solution could be to use Long.MIN_VALUE as special value.

I accept your solution too, let me know what do you prefer.

879f196#commitcomment-3261065

879f196#commitcomment-3261070

879f196#commitcomment-3260958

879f196#commitcomment-3261105

https://github.com/excilys/androidannotations/pull/564/files#r4329001

pyricau · 2013-05-22T18:09:45Z

Thank you for contributing this and for going through all these reviews. I believe this is a very sensitive part of the code, and it's hard to get threading right, which is why everybody gets very sensitive about this.

This code looks good to me. What I'm missing now is more code using this. Ideally we'd need to put that into apps, see what are the use cases, and if that fits with what we usually want to do.

I think it makes sense to bring both pull requests into the code, but we should also start testing that in apps.

Cancel background tasks

DayS · 2013-06-09T19:37:23Z

Thanks for this feature
I made all the tests I could think of and everything seems to work. Great job 👍

rom1v · 2013-06-09T20:45:16Z

Great! Thank you for your time and for merging ;-)

rom1v added 11 commits April 18, 2013 15:12

Test for @background(serial="some_id")

2ded3aa

Test if actions are sequential as expected.

Comment typo fix

d39e77d

Use SystemClock.sleep()

2045e41

Avoid try/catch InterruptedException of Thread.sleep().

Factorize surround with try/catch generation

1ab7373

Extract the surround-with-try-catch part of the anonymous Runnable generation, in order to reuse it for generating other anonymous classes.

Directly generate Task instantiation

ec9fea3

Instead of create a new Runnable instance which will be wrapped by a Task instance, directly create a Task (which is a Runnable).

Cancellable background tasks

99ca871

Adds an optional id attribute to @background: @background(id="some_id") The user is able to cancel all background tasks having a specified id: BackgroundExecutor.cancelAll("some_id");

Tests for @background attributes

0ec12b0

Tests the serialization and cancellation of background tasks.

Do not fail if cancellation is not supported

dbd7caf

If the executor is replaced by another not supporting cancellation (not extending ExecutorService), it failed the first time a task with a non-null id was requested. Now, it just warns during cancellation.

Comment bugfixes

aa2fe11

rom1v mentioned this pull request May 22, 2013

Serial execution in background #564

Closed

rom1v added 5 commits May 22, 2013 16:14

Cast into a local variable before dereferencing

fddcc1b

879f196#commitcomment-3261065

Remove useless comment

33b3cf4

879f196#commitcomment-3261070

Refactor targetTime -> targetTimeMillis

75b1516

879f196#commitcomment-3260958

Refactor delay -> remainingDelay

f3a5718

879f196#commitcomment-3261105

Javadoc for @background annotation

4161c62

https://github.com/excilys/androidannotations/pull/564/files#r4329001

DayS added a commit that referenced this pull request Jun 9, 2013

Merge pull request #569 from rom1v/cancel_background

77cbcea

Cancel background tasks

DayS merged commit 77cbcea into androidannotations:develop Jun 9, 2013

DayS mentioned this pull request Jun 9, 2013

#475 - Rest Authentication #579

Merged

rom1v mentioned this pull request Jun 10, 2013

Serialized @Background task cancellation bugfix #624

Merged

romainpiel mentioned this pull request Jun 10, 2013

New BackgroundExecutor is failing in my app #625

Closed

Bananeweizen mentioned this pull request Jul 6, 2013

Make Handler static in CacheDetailActivity cgeo/cgeo#2931

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cancel background tasks #569

Cancel background tasks #569

Uh oh!

rom1v commented Apr 22, 2013

Uh oh!

melnikovdv commented Apr 26, 2013

Uh oh!

rom1v commented Apr 26, 2013

Uh oh!

rom1v commented May 19, 2013

Uh oh!

gebing commented May 19, 2013

Uh oh!

DayS commented May 21, 2013

Uh oh!

rom1v commented May 22, 2013

Uh oh!

pyricau commented May 22, 2013

Uh oh!

DayS commented Jun 9, 2013

Uh oh!

rom1v commented Jun 9, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Cancel background tasks #569

Cancel background tasks #569

Uh oh!

Conversation

rom1v commented Apr 22, 2013

Uh oh!

melnikovdv commented Apr 26, 2013

Uh oh!

rom1v commented Apr 26, 2013

Uh oh!

rom1v commented May 19, 2013

Uh oh!

gebing commented May 19, 2013

Uh oh!

DayS commented May 21, 2013

Uh oh!

rom1v commented May 22, 2013

Uh oh!

pyricau commented May 22, 2013

Uh oh!

DayS commented Jun 9, 2013

Uh oh!

rom1v commented Jun 9, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants