Confusing API: ScheduledThreadPoolExecutor vs. remove()

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Confusing API: ScheduledThreadPoolExecutor vs. remove()

Holger Hoffstätte-2

While converting an app to util.concurrent I noticed a (more precisely yet
another) rather confusing API inconsistency, this time with
ScheduledThreadPoolExecutor. A Runnable puts itself on the queue and gets
run() every so often - fine, works as expected. It also needs to remove
itself from the queue when a certain condition is met. Surprise: remove()
is overridden from ThreadPoolExecutor and while it accepts a Runnable,
it's really the ScheduledFuture returned from schedule..() that has to be
passed in and *cast to Runnable*. This is the case with both backport and
mustang-b61. Why? The ScheduledFuture interface does not extend Runnable
(that's ok) while the returned object (a RunnableScheduledFuture) does and
can successfully be cast, but all this is just very inconsistent. :-(
I would have expected that if a Runnable can schedule itself that it can
also remove itself (let the queue figure out the wrapper business).
Alternatively, if schedule() returns a wrapper object to the Runnable I
would have expected that I can pass that in without typecasting (it's
impossible to declare the reference 'correctly') or having to read the source.
Are there some efforts underway to fix these API bugs, maybe in future
Mustang builds?

thanks,
Holger

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

tpeierls
Holger Hoffst?tte wrote:
> I would have expected that if a Runnable can schedule itself that it can
> also remove itself (let the queue figure out the wrapper business).

Future.cancel is the usual way to cancel a task. ExecutorService.submit and
ScheduledExecutorService.schedule both return a Future. The remove methods of TPE and STPE are
provided to allow for custom cancellation policies when simple cancellation isn't enough. For example:

   class MyCustomSTPE extends ScheduledThreadPoolExecutor {
       protected <V> RunnableScheduledFuture<V> decorateTask(
               Runnable runnable, RunnableScheduleFuture<V> task) {
           final ScheduledThreadPoolExecutor exec = this;
           return new RunnableScheduledFuture<V>() {
               // delegate to task, except for:
               public void cancel(boolean mayInterruptIfRunning) {
                   task.cancel(mayInterruptIfRunning);
                   exec.remove(this); // try to remove it from queue
               }
           };
       }
   }

--tim



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

Holger Hoffstätte-2
Tim Peierls wrote:
> Holger Hoffst?tte wrote:
>
>> I would have expected that if a Runnable can schedule itself that it can
>> also remove itself (let the queue figure out the wrapper business).
>
> Future.cancel is the usual way to cancel a task. ExecutorService.submit

Thank you for pointing that out - ripping the job from the queue certainly
did feel a little awkward at first glance; cancel() does exactly the right
thing. I knew about it but I guess it did not occur to me that cancelling
such a wrapper would also remove it from the queue. Now everything works
without casting and all objects are happily scheduling themselves. :)

Thanks again!
Holger

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

tpeierls
Holger Hoffst?tte wrote:
> Thank you for pointing that out - ripping the job from the queue certainly
> did feel a little awkward at first glance; cancel() does exactly the right
> thing. I knew about it but I guess it did not occur to me that cancelling
> such a wrapper would also remove it from the queue. Now everything works
> without casting and all objects are happily scheduling themselves. :)

Glad it's working, but just to be sure: cancel doesn't actually remove from the queue, it just
arranges for the runnable not to be run. The code fragment I gave was for situations where it was
important to physically remove the runnable from the queue -- something that would only crop up if
you had a *lot* of runnables in the queue that were getting cancelled and bogging down the
execution service. (Although I'd have my doubts about a design in which this happened.)

--tim



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

Holger Hoffstätte-2
Hi,

Sorry to rehash this but it seems to be a deeper problem.

Tim Peierls wrote:
> Glad it's working, but just to be sure: cancel doesn't actually remove
> from the queue, it just arranges for the runnable not to be run. The

OK I slept over it and now understand what's going on.

> code fragment I gave was for situations where it was important to
> physically remove the runnable from the queue -- something that would
> only crop up if you had a *lot* of runnables in the queue that were
> getting cancelled and bogging down the execution service. (Although I'd
> have my doubts about a design in which this happened.)

The Runnable wants to update its scheduling interval and there's no
interface-based way to do that, so it needs to remove itself before
re-queueing with the updated delay. Easy, right? Yes I know I could use
java.util.Timer but the whole point of the exercise was to create not too
many Threads and migrate existing classes. So..

- I created a CustomSTPE as you outlined

- since I don't want to scatter all the RunnableScheduledFuture delegate
methods in my application I created a skeleton
CustomScheduledRunnableFuture class that wraps the incoming
RunnableScheduledFuture and delegates to it. The app just creates an
anonymous inner instance overriding cancel(), as in the example.

- The end result is a ClassCastException in
ScheduledFutureTask.compareTo() since schedule.. creates a private
ScheduledFutureTask that I can wrap, but passing my wrapper instance to
remove() (as in your example) is wrong since I don't inherit from the
private class (and obviously cannot). compareTo casts to the private class.

- However, passing the wrapped task to remove() works - the wrapper is
correctly removed from the queue - because only one wrapper exists for a
given task and this lets the queue remove the correct object at the found
index, which happens to be my wrapper.

In summary I find the effort required for this simple use case quite
disturbing because it should be much, much easier to accomplish and
achievable with less code, not to mention requiring less "accidental
success" or reading private source code details.
IMHO ScheduledFutureTask should be made public (just like other
interface/class pairs) so that customized behaviour can be attached on a
per-instance-basis (kind of like prototypes) without having to write or
generate endless amounts of wrapper code. STPE.remove() should probably be
made protected so that people don't get the wrong ideas about what it
does, since you effectively have to subclass TPE anyway.

thanks,
Holger
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

tpeierls
Holger Hoffstätte wrote:
> The Runnable wants to update its scheduling interval and there's no
> interface-based way to do that, so it needs to remove itself before
> re-queueing with the updated delay.

But why remove? Why not just cancel and schedule again?


> - The end result is a ClassCastException in
> ScheduledFutureTask.compareTo() since schedule.. creates a private
> ScheduledFutureTask that I can wrap, but passing my wrapper instance to
> remove() (as in your example) is wrong since I don't inherit from the
> private class (and obviously cannot). compareTo casts to the private class.

Thanks for pointing this out! That seems like a bug in the implementation of
ScheduledFutureTask.compareTo. It's being held in a DelayQueue<RunnableScheduleFuture> so it
shouldn't assume that everything is a ScheduledFutureTask.


> In summary I find the effort required for this simple use case quite
> disturbing because it should be much, much easier to accomplish and
> achievable with less code, not to mention requiring less "accidental
> success" or reading private source code details.

I sort of agree in general, but in this case, the complication arises because of the attempt to
remove the runnable from the queue, not a normal thing at all, and something for which extra
effort seems reasonable.


> IMHO ScheduledFutureTask should be made public (just like other
> interface/class pairs) so that customized behaviour can be attached on a
> per-instance-basis (kind of like prototypes) without having to write or
> generate endless amounts of wrapper code.

Or we could provide standard delegating implementations of classes that often need wrapping, like
RunnableScheduledFuture.


> STPE.remove() should probably be
> made protected so that people don't get the wrong ideas about what it
> does, since you effectively have to subclass TPE anyway.

That's a good point, but it's too late now. Once it's public, it has to stay public. (And it would
have to be protected in TPE, too.)

--tim



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

Holger Hoffstätte-2
Tim Peierls wrote:
> Holger Hoffstätte wrote:
>
>> The Runnable wants to update its scheduling interval and there's no
>> interface-based way to do that, so it needs to remove itself before
>> re-queueing with the updated delay.
>
> But why remove? Why not just cancel and schedule again?

But..wouldn't that fill up the queue over time? I also found TPE.purge()
and assume that I could use that with a stock STPE instead of all the
subclassing and removing..looking through the source I could not find any
autopurging, right?

Obviously I'm missing something here but I don't know what it is. :D

>> - The end result is a ClassCastException in
>> ScheduledFutureTask.compareTo() since schedule.. creates a private
>> ScheduledFutureTask that I can wrap, but passing my wrapper instance to
>> remove() (as in your example) is wrong since I don't inherit from the
>> private class (and obviously cannot). compareTo casts to the private
>> class.
>
> Thanks for pointing this out! That seems like a bug in the
> implementation of ScheduledFutureTask.compareTo. It's being held in a
> DelayQueue<RunnableScheduleFuture> so it shouldn't assume that
> everything is a ScheduledFutureTask.

I'm relieved that you agree. Should I file this as a bug, then? It seems
to affect backport, 1.5.0_05 and Mustang-b61.

> Or we could provide standard delegating implementations of classes that
> often need wrapping, like RunnableScheduledFuture.

Yes please!

Thanks again for your help.
Holger

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutorvs.remove()

Dawid Kurzyniec
Holger Hoffstätte wrote:

>Tim Peierls wrote:
>  
>
>>Holger Hoffstätte wrote:
>>
>>    
>>
>>>The Runnable wants to update its scheduling interval and there's no
>>>interface-based way to do that, so it needs to remove itself before
>>>re-queueing with the updated delay.
>>>      
>>>
>>But why remove? Why not just cancel and schedule again?
>>    
>>
>
>But..wouldn't that fill up the queue over time? I also found TPE.purge()
>and assume that I could use that with a stock STPE instead of all the
>subclassing and removing..looking through the source I could not find any
>autopurging, right?
>
>Obviously I'm missing something here but I don't know what it is. :D
>  
>
The stale entries are removed as they approach the head of the list and
get extracted by the executor. So, in the long term, there is no
accumulation. But yes, this can cause memory problems, especially if you
use ScheduledThreadPool with relatively large delays.

I have run into this problem when working on a leasing-based naming
service. When entry is added to the NS, I schedule a cleanup task,
responsible for removing the entry after lease expires, unless the lease
has been renewed. If the lease has been renewed, or if the entry has
been removed and possibly re-added, that old cleanup task realizes it
upon invocation and does nothing. (If the entry was overwritten or
re-added, there is a new cleaner scheduled already). There is a problem,
however, if entries are frequently changed, in which case many of those
cleaner tasks are created. So, I started cancelling that old cleaner
tasks upon remove or overwrite. But, as you noted, cancelled tasks are
not immediately removed from the queue. If leases are long, which they
may be, this poses a serious memory leak.

Removing the task upon cancel is a bad idea, however, because it
requires a linear search.

The best solution I came up with is to purge once every some fixed
number of cancels. To count cancels, you can use an atomic integer. Upon
cancel, you decrementAndGet, and if you reached 0, you reset to some
number (e.g. 10000) and purge. This guarantees that your queue will
never had more than a fixed number of garbage entries. (10000 seems to
me a reasonable compromise between memory footprint and purging frequency).

Regards,
Dawid Kurzyniec


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

tpeierls
In reply to this post by Holger Hoffstätte-2
Holger Hoffstätte wrote:
>>>The Runnable wants to update its scheduling interval and there's no
>>>interface-based way to do that, so it needs to remove itself before
>>>re-queueing with the updated delay.
>>
>>But why remove? Why not just cancel and schedule again?
>
> But..wouldn't that fill up the queue over time?

The underlying queue is unbounded, the wrapper objects are tiny, and the queue is drained by the
pool (cancelled tasks are consumed silently). And, as you observed, you can purge the cancelled
tasks if they start to build up.


> I also found TPE.purge()
> and assume that I could use that with a stock STPE instead of all the
> subclassing and removing..looking through the source I could not find any
> autopurging, right?

No autopurging, and purge is almost certain to be linear in the size of the queue, so probably not
something you want to do all the time. But you could schedule a periodic task to purge often
enough to ensure that cancelled tasks don't clutter the queue, but not so often that purging gets
in the way of real work.



>>>- The end result is a ClassCastException in
>>>ScheduledFutureTask.compareTo()  ... compareTo casts to the private class.
>>
>>Thanks for pointing this out! That seems like a bug in the
>>implementation of ScheduledFutureTask.compareTo. It's being held in a
>>DelayQueue<RunnableScheduleFuture> so it shouldn't assume that
>>everything is a ScheduledFutureTask.
>
> I'm relieved that you agree. Should I file this as a bug, then? It seems
> to affect backport, 1.5.0_05 and Mustang-b61.

I'm hoping that someone who knows how to deal with bugs will chime in and agree that it's a bug
and take care of it. :-)


>>Or we could provide standard delegating implementations of classes that
>>often need wrapping, like RunnableScheduledFuture.
>
> Yes please!

Wish list:

- DelegatingRunnableFuture
- DelegatingRunnableScheduledFuture
- SimpleAbstractExecutorService (need better name, lifecycle methods given
       trivial implementations so all you need to define is execute)
- what else?

--tim



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Confusing API: ScheduledThreadPoolExecutor vs. remove()

Doug Lea
Tim Peierls wrote:

> Holger Hoffstätte wrote:
>>>>
>>>> - The end result is a ClassCastException in
>>>> ScheduledFutureTask.compareTo()  ... compareTo casts to the private
>>>> class.
>>>
>>> Thanks for pointing this out! That seems like a bug in the
>>> implementation of ScheduledFutureTask.compareTo. It's being held in a
>>> DelayQueue<RunnableScheduleFuture> so it shouldn't assume that
>>> everything is a ScheduledFutureTask.

Holger - thanks! We've replaced the compareTo with a version
that can cope with arbitrary Delayed's if the argument is
not a ScheduledFutureTask. This will hopefully make it in Mustang.

-Doug



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest