I agree with your approach with the WeakRunnable; I think that will achieve the goal without the performance hit of calling purge() after each cancellation.
If you take on the synchronized keywords, you should consider whether we're worried about concurrent calls to doStart() and doStop(). That scenario could leak a Timer, and the current code doesn't protect against that (you'd need a flag to say whether you've been stopped, and you'd need to check it in doStart() ). It's the only reason I see that we might want to synchronize those two methods. For the other three, make sure Timer itself is threadsafe, not just the Map, before you remove the synchronized keywords. Tim On May 2, 2015 5:42 PM, "Kevin Burton" <bur...@spinn3r.com> wrote: > Wanted some feedback on this. > > https://gist.github.com/burtonator/34a67c24ca9ce0574c04 > > I think I want to refactor the cancel method… > > it calls purge() which is VERY expensive on large numbers of queues. N^2 > expensive. > > once the cancel() is called, the timer task won’t get executed, HOWEVER > there’s still a reference to the Runnable. This is why I think purge() is > called. This allows GC to kick in and remove the runnable without waiting > for the timer to fire. Which I think is just 30 seconds later but not > sure. > > So I think the workaround here is to not to submit the Runnable itself but > a WeakRunnable and then, once we read the TimerTask, we just set the > reference to the underlying runnable to null. This will allow GC and we > also don’t have to call purge(). > > Ps. There are some other things that should be cleaned up here. Probably > removing synchronized and using ConcurrentHashMap and also probably > ditching Timer and moving to Executors.. but that’s another project in and > of itself and might introduce bugs for no win. > > Kevin > > -- > > Founder/CEO Spinn3r.com > Location: *San Francisco, CA* > blog: http://burtonator.wordpress.com > … or check out my Google+ profile > <https://plus.google.com/102718274791889610666/posts> >
