RingBufferLogHandler.records could leak records

[jglick]

As @PierreBtz points out in jenkinsci/support-core-plugin#324 (review), if someone set up a custom logger that recorded messages from Pipeline components, it could temporarily hold on to GroovyClassLoaders from completed builds, which is not something we want as these might be quite large; seems preferable to let log messages be dropped under extreme memory pressure conditions. Perhaps it is possible to use PhantomReference to replace a record with its formatted equivalent just before collection but this seems like overengineering.

Note that in both the core and plugin cases, it is not exactly correct to hold a list of unformatted LogRecords at all, when they are not going to be immediately written to some log sink. For example code like this

AtomicInteger x = new AtomicInteger();
void doSomething() {
    LOGGER.log(Level.FINE, "running with x={0}", x);
    use(x.incrementAndGet());
}

would send messages like

running with x=0
running with x=1
running with x=2

to a log file, but display

running with x=3
running with x=3
running with x=3

in a GUI display. Unfortunately there is no way to know for sure if the sub-INFO record is going to ever be formatted, so formatting eagerly in publish would usually be wasteful just in order to properly handle log messages from code using this antipattern (log record parameters whose toString depends on mutable state).

I am also cleaning up some concurrency code that looked dubious in f999510 and more so in #5772. Since accesses to start and size are synchronized, why use additional constructs? Perhaps this made sense when returning a lazily evaluated list, though I am not convinced its implementation was ever correct. Simpler and clearer to just construct a concrete list of results if and when getView is called, which is done for example in

jenkins/core/src/main/resources/hudson/logging/LogRecorder/index.jelly

Line 34 in 71117cc

<t:logRecords logRecords="${it.logRecords}"/>

Proposed changelog entries

• Custom log records with large record parameters no longer interfere with garbage collection.

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[jglick]

Strengthening test to assert order as well.

[timja]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[timja]

git bisect identifies this as the revision that has broken ATH (picked up in jenkinsci/acceptance-test-harness#716).

b6cec365302c88143c80ec2c42f82deb99d81865 is the first bad commit
commit b6cec365302c88143c80ec2c42f82deb99d81865
Author: Jesse Glick <[email protected]>
Date:   Tue Dec 7 17:49:38 2021 -0500

    `RingBufferLogHandler.records` could leak records (#6018)

 .../java/hudson/util/RingBufferLogHandler.java     | 49 +++++++++++-----------
 .../test/java/hudson/logging/LogRecorderTest.java  |  9 ++--
 2 files changed, 28 insertions(+), 30 deletions(-)

System log no longer works after this change: