Skip to content
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

Serial execution in background #564

Closed
wants to merge 5 commits into from

Conversation

@rom1v
Copy link
Contributor

@rom1v rom1v commented Apr 18, 2013

Inspired by the ideas of #309, I propose an alternative implementation for providing a serialized execution of background tasks.

The purpose is to add an optional serial attribute to @Background

@Background(serial="some_id")

so that calls to @Background annotated method having the same non-null serial are guaranteed to be executed one at a time, in the order (but not necessarily in 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.

Of course, if two methods use a different serial, they are independant (no serial execution constraint) and will be executed in parallel.

Please don't hesitate if you have any questions or feedbacks.

rom1v added 5 commits Apr 18, 2013
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.
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().
@rom1v rom1v mentioned this pull request Apr 22, 2013
private static final Set<String> serialRunning = new HashSet<String>();

/* Tasks queues for each serial */
private static final Map<String, List<Task>> serialQueues = new HashMap<String, List<Task>>();
Copy link
Contributor

@mathieuboniface mathieuboniface May 21, 2013

java.util.List is not a queue, please use java.util.Queue in this purpose.

Copy link
Contributor Author

@rom1v rom1v May 22, 2013

This is debatable.

In theory, you're right. However, the Java API does not provide a queue implementation backed by an array (except for concurrency).
If the queue is intended to contain only very few items, a LinkedList would be less efficient (more instantiations and references).

OK, we don't care, it's fast enough, but that is the reason why I used an ArrayList. I would have accepted to use a Queue (like a LinkedList).

But anyway, it was removed by 5254f56, and the new variable tasks is a list, not a queue.

@mathieuboniface
Copy link
Contributor

@mathieuboniface mathieuboniface commented May 21, 2013

Hi @rom1v,

Thank you for helping AndroidAnnotations.

To me, this PR introduce a lot of complexity (Semaphore, Task scheduling, synchronized blocks, etc...) and the purpose of AA is to make the use of Android API more easy. I'm not sure this feature will help developers to increase the readability of their source code and so to increase the quality of their apps.

You have added a test and this is really important in AA, however i'm not sure that only one test is enough to make sure all the things are going in the right way.

For now, i'm thinking that we should not provide such feature because AA cannot provide a solution for all the needs around threading. In fact, AA provide a simple solution for simple needs. If developers need specific features as serial threading, they can simply implement their own solution.

Finally i'm also sure that this kind of features could be provided by a separate open source project ;)

@pyricau @JoanZapata any thought ?

@JoanZapata
Copy link
Contributor

@JoanZapata JoanZapata commented May 22, 2013

@mathieuboniface He used semaphore and synchronized blocks only for tests, it sounds good to me, though it might need some more tests to cover at least all the nominal usecases. (using different serial values, mixing serial and parallel execution, etc...)

This feature is a good one IMO, and I think it's pretty straighforward to explain in the wiki (moreover serial is optional).

@mathieuboniface
Copy link
Contributor

@mathieuboniface mathieuboniface commented May 22, 2013

@JoanZapata synchronized blocks are also in the api file. Take a look here and here.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented May 22, 2013

Hi,

Thank you for your review.

I will consider your individual comments, but first I would like to make a precision and a general comment.

Pull requests dependencies

I proposed two features, serial background (this one) and Cancel background tasks. They are dependent: I wrote the second over the first. As you can see, the first commits of the latter are the one of the former (I generalized the tasks management for both feature to share a large part the implementation).

I agree this can be confusing, but I don't see a better way to propose two different features with dependencies in the implementation, especially once the first has already been submitted.

Thus, this pull request in "included" in the other. Some of your comments do still apply to the new implementation, but some don't apply anymore. I will make the changes you request over the last commit.

General comments

To me, this PR introduce a lot of complexity (Semaphore, Task scheduling, synchronized blocks, etc...) and the purpose of AA is to make the use of Android API more easy.

As @JoanZapata pointed out, the concurrency tools are used essentially in functional tests.

The main code contains only mutex (synchronized blocks), quite trivial, which seem inevitable for a multi-threading feature. They concern only the BackgroundExecutor implementation, and do not change the use of AA at all.

I'm not sure this feature will help developers to increase the readability of their source code and so to increase the quality of their apps.

From a AA user point of view, my pull requests only adds two optional arguments to @Background annotation, nothing else. So it does not make their source code more complex. In fact, my goal was precisely to simplify some use cases.

It does not even make the generated code more complex: it just calls BackgroundExecutor.execute(…) with the right parameters.

You have added a test and this is really important in AA, however i'm not sure that only one test is enough to make sure all the things are going in the right way.

There are more tests for interaction between serial and cancellation features.

If you think of other cases to test, do not hesitate.

For now, i'm thinking that we should not provide such feature because AA cannot provide a solution for all the needs around threading. In fact, AA provide a simple solution for simple needs. If developers need specific features as serial threading, they can simply implement their own solution.

In my opinion, serializing background tasks is not a marginal need: every time a @Background-annotated method task modify a shared state, then tasks must potentially be serialized. For example, imagine a checkbox which modify a flag in a database: I/O access to the database must occur in background, but the first click must not override the second, it would make the state inconsistent.

If a user don't use AA, then they will always use Threads/AsyncTasks/… for their background tasks. That way, it is very easy for them to cancel any task. The convenient @Background annotation did not provide this possibility: if someone use @Background and they want to add the ability to cancel it, typically in onPause() or onStop() (which is a very common case), they had to rewrite their code without using AA, in order to get a cancellable object. Now, they could just add and id to the annotation and cancel when they want.

That's why I think these features have their place in AA (whatever the implementation is).

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented May 22, 2013

I commited the changes you requested (and answered directly in the comments).

@pyricau
Copy link
Contributor

@pyricau pyricau commented May 22, 2013

Sorry for coming late into the party. I haven't read the code yet, because when I tried, I realized that you already took some feedback into account, fixed it in specific commits, but didn't update the branch this pull request relies on. That's the way PRs are supposed to work : once you get feedback, you add commits directly to the branch that the pull request is linked to. Then, GitHub automatically closes comments that are one lines that have been modified by a new commit.

@rom1v
Copy link
Contributor Author

@rom1v rom1v commented May 22, 2013

@pyricau

That's the way PRs are supposed to work : once you get feedback, you add commits directly to the branch that the pull request is linked to. Then, GitHub automatically closes comments that are one lines that have been modified by a new commit.

Absolutely.

But as explained here, this is because I did two different pull requests for two different features, with a dependent implementation.

I first did the "serial" feature (this PR), then for implementing task cancellation I improved the implementation, which now works both for serial background and task cancellation. Thus, it would not be pertinent to fix the "old" implementation of serial (this pull request) and then merge into the new (with conflicts), but directly fix the "new" one (=old+changes) instead.

In theory, this one should be forgotten: the other contains both.

@mathieuboniface
Copy link
Contributor

@mathieuboniface mathieuboniface commented May 22, 2013

After a discussion with @pyricau and also because i'm feeling a bit alone on this. I think you can merge that PR.

@DayS @JoanZapata can you continue the review ?

@DayS
Copy link
Contributor

@DayS DayS commented May 22, 2013

Yep. But I don't know if I could review the rest of this PR until next week. And as @pyricau said on the other PR, we really have to test this a lot

@JoanZapata
Copy link
Contributor

@JoanZapata JoanZapata commented May 23, 2013

Same for me, I won't have time until next week.
Until then, is everyone ok with closing this pull request and focus on the new one, as @rom1v suggested ?
@pyricau @mathieuboniface @DayS

@DayS
Copy link
Contributor

@DayS DayS commented May 23, 2013

Yep, it'll be less confusing

@DayS DayS closed this May 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants