New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel background tasks #569

Merged
merged 17 commits into from Jun 9, 2013

Conversation

Projects
None yet
6 participants
@rom1v
Contributor

rom1v commented Apr 22, 2013

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.

rom1v added some commits Apr 18, 2013

Immediate and delayed executors unification
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.
Serial execution in background feature
Adds an optional serial attribute to @background:

    @background(serial="some_id")

This idea comes from #309:
#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 for @background(serial="some_id")
Test if actions are sequential as expected.
Use SystemClock.sleep()
Avoid try/catch InterruptedException of Thread.sleep().
Better task manager implementation
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.
Factorize surround with try/catch generation
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
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
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
Tests the serialization and cancellation of background tasks.
Do not fail if cancellation is not supported
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

This comment has been minimized.

melnikovdv commented Apr 26, 2013

Very cool, thx

@rom1v

This comment has been minimized.

Contributor

rom1v commented Apr 26, 2013

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

@rom1v

This comment has been minimized.

Contributor

rom1v commented May 19, 2013

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

This comment has been minimized.

Contributor

gebing commented May 19, 2013

sorry, it is my fault to compile with duplicate jars

@DayS

This comment has been minimized.

Contributor

DayS commented May 21, 2013

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

@DayS

This comment has been minimized.

isEmpty is only available from API 9. We should be careful with that be (at least) compatible with SDK 8

This comment has been minimized.

mathieuboniface replied May 21, 2013

I agree with @DayS, isEmpty() have to be replaced.

This comment has been minimized.

mathieuboniface replied May 21, 2013

serial cannot be null. No need to test if it's equal to null.
You cannot write @Background(serial=null)because the compiler throw an error.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

isEmpty is only available from API 9. We should be careful with that be (at least) compatible with SDK 8

Thank you, I didn't know that. I was convinced it was included for longer.

However, this test has been removed in commit 5254f56.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

serial cannot be null. No need to test if it's equal to null.
You cannot write @Background(serial=null) because the compiler throw an error.

This method is called with null parameter here for example.

The idea is to have only one method execute, with all (potentially null or 0) parameters. With the cancellation feature, the method becomes:

public static void execute(final Runnable runnable, String id, int delay, String serial)
@DayS

This comment has been minimized.

DayS commented on 879f196 May 21, 2013

I made this stupid code to test your feature :

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    for (int i = 0; i < 10; i++)
        addSerializedBackgroundMethod(i);
}

@Background(serial = "test")
void addSerializedBackgroundMethod(int i) {
    SystemClock.sleep(new Random().nextInt(2000)+1000);
    Log.d("SANDBOX", "value : " + i);
}

But I only had one log... It seems that the computed delay is always near of Long.MAX_VALUE because it wasn't initiated

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()));
}

Tell me if it makes sense to you.

Apart from that, everything seems great for me.
@mathieuboniface @pyricau @JoanZapata ?

@mathieuboniface

This comment has been minimized.

Please replace targetTime by targetTimeMillis and remove this comment

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Done in commit 75b1516.

@mathieuboniface

This comment has been minimized.

Please remove that comment, it does not provide relevant informations

This comment has been minimized.

Owner

rom1v replied May 22, 2013

This comment was removed by 5254f56.

@mathieuboniface

This comment has been minimized.

Please use local vars to facilitate application debugging and code readability.

ScheduledExecutorService scheduledExecutor = ((ScheduledExecutorService) executor)
scheduledExecutor.schedule(runnable, delay, TimeUnit.MILLISECONDS)

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Done in commit fddcc1b.

@mathieuboniface

This comment has been minimized.

This comment is useless, it does not provide relevant informations, please remove it.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Done in commit 33b3cf4.

@mathieuboniface

This comment has been minimized.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Was removed by 5254f56.

@mathieuboniface

This comment has been minimized.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Was removed by 5254f56.

@mathieuboniface

This comment has been minimized.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Was removed by 5254f56.

@mathieuboniface

This comment has been minimized.

You are repeating the line just below, please remove that comment

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Was removed by 5254f56.

@mathieuboniface

This comment has been minimized.

Ok, so rename delay to remainingDelay and remove that comment :)

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Done in commit f3a5718.

@mathieuboniface

This comment has been minimized.

We already see that by reading the line just above, please remove that comment.

This comment has been minimized.

Owner

rom1v replied May 22, 2013

Was removed by 5254f56.

@rom1v

This comment has been minimized.

Contributor

rom1v commented May 22, 2013

@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.

@pyricau

This comment has been minimized.

Contributor

pyricau commented May 22, 2013

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.

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

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

@DayS

This comment has been minimized.

Contributor

DayS commented Jun 9, 2013

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

@rom1v

This comment has been minimized.

Contributor

rom1v commented Jun 9, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment