Skip to content

Add delay in @Background #525

Merged
merged 4 commits into from Apr 13, 2013

5 participants

@yDelouis
yDelouis commented Mar 4, 2013

Hi,

This commit is a solution for the issue #332.

Because the class Executor does not have a method executeDelayed, I use a Handler to postDelayed the call to the method execute.

Comments are very welcome.

@pyricau
excilys member
pyricau commented Mar 5, 2013

Hi,

Thank you for contributing this. However, I don't think you need to use a Handler this. The JDK already has the needed bits : http://docs.oracle.com/javase/1.5.0/docs/api/java/util/concurrent/ScheduledExecutorService.html

@yDelouis
yDelouis commented Mar 5, 2013

I didn't know about ExecutorService.
I'll change that soon.

@yDelouis
yDelouis commented Mar 9, 2013

I switched from Handler to ScheduledExcutorService.

@pyricau pyricau and 4 others commented on an outdated diff Mar 9, 2013
...va/org/androidannotations/api/BackgroundExecutor.java
public class BackgroundExecutor {
private static Executor executor = Executors.newCachedThreadPool();
+ private static ScheduledExecutorService scheduledExecutor = Executors.newScheduledThreadPool(1);
@pyricau
excilys member
pyricau added a note Mar 9, 2013

The core pool size set to 1, which AFAIK means the maximum number of threads on this executor will always 1. As a consequence, you'll never get two delayed tasks to run concurrently. Is that on purpose ? @Background has always had a parallel behavior, if we want serial behavior which should make a clear distinction.

@yDelouis
yDelouis added a note Mar 11, 2013

I didn't know which value to set here. Maybe a big one (ex: Integer.MAX_VALUE) or maybe we should create a ScheduledExecutorService each time we call executeDelayed ? Do you know the effect of a value of 0 for the core pool size ? It's not said in the java doc and it doesn't seem to rise an exception.

@ThomasFondrillon
excilys member

It may be a good thing to let the user defines how many threads he wants because this is typically a parameter that changes according to the needs.

Maybe with another optional parameter in the @Background annotation. If the user doesn't fill this parameter, assign a default value to 5 for the corePoolSize.

What do you think about this idea?

@mathieuboniface
excilys member

I don't think it's a good idea as it means each @Background could be responsible of the creation of a thread pool. That could cause a performance issue, and it could be hard to understand from a developer point of view.

I would prefer to define a value. Something like that :

What do you think ?

@yDelouis
yDelouis added a note Mar 12, 2013

I think Mathieu is right. The more so as a developper will be able to change the ScheduledExecutorService using the static method of BackgroundExecutor. Regarding the value to set, I have no idea.

@DayS
excilys member
DayS added a note Mar 12, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pyricau pyricau and 1 other commented on an outdated diff Mar 9, 2013
...org/androidannotations/helper/APTCodeModelHelper.java
@@ -228,6 +228,19 @@ public String getIdStringFromIdFieldRef(JFieldRef idRef) {
throw new IllegalStateException("Unable to extract target name from JFieldRef");
}
+ public JDefinedClass createAnonymousRunnableClass(EBeanHolder holder, JStatement block) {
@pyricau
excilys member
pyricau added a note Mar 9, 2013

Why do we need to introduce a new method instead of using createDelegatingAnonymousRunnableClass() ?

@mathieuboniface
excilys member

This method seems to be not used, can you delete this code ?

@mathieuboniface
excilys member

Wow, same time :)

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

I changed the corePoolSize.

@mathieuboniface mathieuboniface merged commit a1f430a into excilys:develop Apr 13, 2013
@mathieuboniface
excilys member

Great work 👍

@yDelouis yDelouis deleted the yDelouis:332_DelayInBackground branch Apr 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.