[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]

:bee:

[tfennelly]

:bee:

[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]

:bee:

[jglick]

:bee: 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.