[JENKINS-68122] Avoid deadlock involving RingBufferLogHandler.LogRecordRef class loading (II)

[jglick]

See JENKINS-68122. Amends #6018 + #6044. Alternative to #6444.

Effectiveness still being validated. Suggested by @roband7 (hope the GitHub and Jira ids align). I think the problem is in

jenkins/core/src/main/java/hudson/slaves/SlaveComputer.java

Lines 1062 to 1092 in 71d5dd5

	/**
	* Puts the {@link #SLAVE_LOG_HANDLER} into a separate class so that loading this class
	* in JVM doesn't end up loading tons of additional classes.
	*/
	static final class LogHolder {
	/**
	* This field is used on each agent to record logs on the agent.
	*/
	static RingBufferLogHandler SLAVE_LOG_HANDLER;
	}
	private static class SlaveInitializer extends MasterToSlaveCallable<Void, RuntimeException> {
	final int ringBufferSize;
	SlaveInitializer(int ringBufferSize) {
	this.ringBufferSize = ringBufferSize;
	}
	@Override
	@SuppressFBWarnings(value = "ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", justification = "field is static for the reason explained in the Javadoc for LogHolder")
	public Void call() {
	SLAVE_LOG_HANDLER = new RingBufferLogHandler(ringBufferSize);
	// avoid double installation of the handler. Inbound agents can reconnect to the controller multiple times
	// and each connection gets a different RemoteClassLoader, so we need to evict them by class name,
	// not by their identity.
	for (Handler h : LOGGER.getHandlers()) {
	if (h.getClass().getName().equals(SLAVE_LOG_HANDLER.getClass().getName()))
	LOGGER.removeHandler(h);
	}
	LOGGER.addHandler(SLAVE_LOG_HANDLER);

basil@38efb0b is another variant.

jenkinsci/remoting#527 might offer a hotfix for those who cannot easily change the core (controller) version.

Proposed changelog entries

• Avoid a deadlock between agent class loading and logging.

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 accurate, human-readable, and in the imperative mood
• 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).

[basil]

As explained in the bug, I have a slight preference for doing this in the static initializer than in the constructor as in basil@38efb0b, but I do not feel strongly about this.

I have a strong preference for adding an explanatory comment (e.g., the "Preload the LogRecordRef class…" comment from basil@38efb0b), as without such a comment future readers may not understand the reason why this is necessary.

[jglick]

I have a slight preference for…

Just file your commit then. Happy to approve it. Most important is field validation that whichever fix works.

[jglick]

(and getting this in trunk soon so that it makes it into 2.332.3)

[basil]

Agreed that there is no need to delay this further based on a mild preference. I am not going to file a PR.

It would be nice to get a response to my feedback regarding the explanatory comment.

[jglick]

Just filed your commit as #6449—simpler than negotiating text of a comment.

[basil]

My point is that the comment should not only state what we are doing (i.e., preloading the class) but also why we are doing it (i.e., to avoid a deadlock), because in my opinion this is not obvious and therefore likely to be misunderstood in the future by someone without the context that we now have.

[jglick]

True, it is not obvious and going into git blame can be tedious. Forcing eager class loading to prevent a deadlock with a custom logger is one of those things you need to do occasionally in modular Java systems but it counts as esoteric. (https://github.com/jenkinsci/lib-support-log-formatter/blob/80a5636c8924e0984cd7a0ac8fb8ae57fe4e6b75/src/main/java/io/jenkins/lib/support_log_formatter/SupportLogFormatter.java#L27 is another example)

[basil]

The operative word in that example is "because", indicating that causal reasoning is about to follow. I suggest we add that causal reasoning to a comment in this PR.

[jglick]

#6449 was specifically tested so let us go with that.