[JENKINS-70103] Clear websocket sessions on connection close or error

[amuniz]

This is causing a memory leak, as sessions pile up there and the GC does not collect them.

See JENKINS-70103 for more information.

I'll file a follow up PR in remoting to slow down connection re-try at agent side. There is no point on trying hundreds of times per second.

Testing done

AFAICT the current test setup is not ready to test an agent running on Java 8 trying to connect to Jenkins on Java 11, so I did some manual tests:

1. Run Jenkins on Java 11.
2. Create an agent using websocket connection.
3. Run the agent (an older version that supports Java 8) on Java 8.
4. The agent connects and immediately disconnects (because of the Java version mismatch). There are hundreds of connection re-tries per second.
5. Kill the agent process.
6. Using the Jenkins script console verify that jenkins.websocket.Jetty10Provider.sessions is clear.

Proposed changelog entries

• [JENKINS-70103] Clear websocket sessions on agent connection close or error, otherwise sessions are accumulated causing a memory leak.

Proposed upgrade guidelines

N/A

Submitter checklist

• The Jira issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
  • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
• New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/core-pr-reviewers

Maintainer checklist

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

• There are at least two (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 is not blocking the change.
• Changelog entries in the pull request 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, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see 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).

[Vlatombe]

LGTM

[jglick]

Fine so far as it goes but you are leaving

jenkins/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java

Line 53 in 206f7f1

private static final Map<Listener, Session> sessions = Collections.synchronizedMap(new WeakHashMap<>());

which is wrong. Either the value refers to the key (please provide a reference chain dump), which would explain the bug, in which case please switch to a plain HashMap or IdentityHashMap for clarity; or it does not, in which case the bug has not been explained.

[jglick]

How do you know this will be called before

jenkins/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java

Lines 99 to 100 in 77c2450

	public void close() throws IOException {
	session().close();

?

[amuniz]

I don't know. Do you have some insight?

[jglick]

No. You would need to test it.

[amuniz]

the value refers to the key

AFAICT by inspecting a heap dump, that's the case. Reference chain is:

The referent (key) in the WeakHashMap#Entry is referenced down the chain from the value (a WebSocketSession)

[jglick]

Then cease use of WeakHashMap as it cannot work.

Ideally we could reproduce this, say in JNLPLauncherRealTest.webSocket, and use MemoryAssert to verify that the session map is cleared as expected.

[jglick]

The problem is

jenkins/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java

Line 52 in 03c380f

// TODO does not seem possible to use HttpServletRequest.get/setAttribute for this

jenkins/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java

Line 61 in 03c380f

req.setAttribute(ATTR_LISTENER, listener);

jenkins/websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java

Line 132 in 03c380f

Listener listener = (Listener) req.getHttpServletRequest().getAttribute(ATTR_LISTENER);

If we cannot use get/setAttribute to look up the Session for a given HttpServletRequest, perhaps it works to use a WeakHashMap keyed by HttpServletRequest?

[jglick]

Nope:

storing session in org.eclipse.jetty.websocket.server.internal.DelegatedServerUpgradeRequest@5ebb565a with attrs [org.kohsuke.stapler.compression.CompressionServletResponse, jenkins.websocket.Jetty10Provider.listener, org.eclipse.jetty.server.HttpTransport.UPGRADE, org.kohsuke.stapler.EvaluationTrace, org.kohsuke.stapler.compression.CompressionFilter]

vs.

looking up session in org.kohsuke.stapler.RequestImpl@7b73cc65 with attrs []

Still looking into options.

[jglick]

I think I found a better approach.

[basil]

Closing in favor of #7412.