[JENKINS-70414] Missing agent-side Channel.close
from PingThread.onDead
#7580
+8
−0
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See JENKINS-70414.
As of jenkinsci/remoting#85, a
PingThread
setup built into Remoting was disabled by default as it duplicated core’sChannelPinger
which already set upPingThread
in both directions.A year later, #3005 improved the behavior of the
PingThread.onDead
handler on the controller side. As described in #3005 (comment)Unfortunately this change removed a call to
Channel.close
that had previously been done on both sides, and only replaced it on the controller side (thecomputer != null
case). On the agent side, the result was that thePingThread
was entirely useless.Testing done
Built trunk WAR. Ran with
Logged in, installed no plugins. Defined a static inbound WebSocket agent. Connected from a local Java process according to suggestions.
At this point if you
then the agent notices as expected, since the TCP socket is closed, and goes into its usual reconnection loop.
However if you instead
then the pinger notices the outage but does nothing:
With this patch, the agent disconnects cleanly, as expected:
and is able to reconnect if and when the controller is again available.
Proposed changelog entries
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).