[FIXED JENKINS-31627] canceled TimerTasks should be purged.

[jtnord]

The TimerTask could be way off into the future as it is user input that
decides when this is run (and or jobs running multiple days the inactivity
could be 1 day). Without purging the Timer the TimerTasks stay into the
queue until such time that they are the next task to run - at which time
they are removed. But with all other triggers in a system (polling every
minute etc) added timers could stay for their maximum duration.

For arguments sake say this is 1 day and you have a build that produces
10000 lines of output every 10 hours and uses the NoActivity strategy.
In this case for every call to write you will have a new TimerTask created
that should trigger in 24hours time - it is easy to see these 10000
TimerTasks are referenced from the Timer and as such will not be garbage
collected for a long while.
If you have lots of these styles of jobs - the required heap to run this
becomes massive.

@reviewbybees
JENKINS-31627

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[jtnord]

The Timer internals are all private (who would have expected anything else) so ran out of good ideas to write a unit test for this - the thought of using reflection entered my head but the insides are likely to change between releases so seemed overly fragile. But the code is trivial - and the fix has been verified by hand and heap dumps.

[rsandell]

🐝

[tfennelly]

I don't get the connection between Trigger.timer.purge() and the task being canceled. Is Trigger.timer a ThreadLocal that has some relevant context that allows it to purge the correct TimerTask?

[tfennelly]

Or maybe it pruges all canceled tasks?

[rsandell]

https://docs.oracle.com/javase/1.5.0/docs/api/java/util/Timer.html#purge%28%29

[jtnord]

@tfennelly
Task is a java.util.TimerTask that is in a java.util.Timer

You cancel the task so that when the timer comes to run it it won't run it - but the timer still holds a reference to it.

[jtnord]

@tfennelly calling purge tells the timer to walk through the list of Tasks it has and remove any that are cancelled.

Its not as cheap as inserting new tasks - or letting them natrually drop of when they come to run - but the instance that showed this issue had * over 3 million* of these objects - and there should be only a few hundred or so. So whilst calling purge() can be expensive - it works out cheaper than letting them pile up and running out of memory.

[tfennelly]

🐝

[jtnord]

@reviewbybees done

[ghost]

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

[stephenc]

🐝

[jglick]

🐝 but note that Trigger.timer has been deprecated in favor of Timer for a while now. Perhaps the modern java.util.concurrent APIs do not suffer from this awful flaw to begin with?

[jtnord]

@jglick ScheduledThreadPoolExecutor does not appear to have a similar flaw - but the Jenkins version that introduced this (1.541) is way later than the very old (1.466) version of Jenkins that this plugin is targeting.

[ikedam]

Thanks for the important fix.

[ikedam]

Tested with Execute shell

#!bash

for i in $(seq 300); do
  for j in $(seq 65535); do
    echo ${i} ${j}
  done
done

	Execution Time	# of TimeoutTimerTask (after execution)	# of TimeoutTimerTask (after GC)
without the fix	5 min 38 sec	11,310,919	7,552,750
with the fix	5 min 23 sec	1,124,989	0

(Counted the # of objects with jmap tool)
I saw that the problem was fixed.
This change doesn't cause a performance issue.