Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JENKINS-67928 Prevent infinite loop in case of closed SSL connection #513

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

gulyaev13
Copy link
Contributor

@gulyaev13 gulyaev13 commented Feb 22, 2022

Background
I've found increased CPU load at my Jenkins instance.
The root cause of CPU load was a thread that is stuck in an infinite loop in SSLEngineFilterLayer.java
The remote agent was already terminated by kubernetes-plugin and the SSL connection is closed, but the thread is still working because can't exit from the while loop.

Solution
Set done flag in case of receiving SSLEngineResult.Status.CLOSED from sslEngine.unwrap(tempBuffer, appBuffer).
This change is allowed to return execution flow to NIONetworkLayer.ready() where the thread will handle ClosedChannelException.

Libraries versions
Jenkins core - 2.319.3
kubernetes - 1.31.3
remoting - 4.11.2

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jglick jglick removed their request for review February 24, 2022 16:30
@jglick
Copy link
Member

jglick commented Feb 24, 2022

I worked on the WebSocket transport but am not experienced in the original TCP/NIO transport.

@timja
Copy link
Member

timja commented Feb 24, 2022

Have you been able to test this yourself and see if it works?

No experience with this protocol

@gulyaev13
Copy link
Contributor Author

@jglick, @timja, I've patched the remoting version via custom-war-packager and tested change at my local instance.

war:
  groupId: "org.jenkins-ci.main"
  artifactId: "jenkins-war"
  source:
    version: 2.319.3
libPatches:
  - groupId: "org.jenkins-ci.main"
    artifactId: "remoting"
    source:
      git: https://github.com/gulyaev13/remoting.git
      commit: b2a29ed6e5445e89d2aaf5de5fa08afd2ef46e5c

CPU utilization with remoting 4.11.2:
Снимок экрана 2022-02-28 в 19 17 37

CPU utilization with my patch for remoting:
Снимок экрана 2022-02-28 в 19 59 24

As you can see there are several peaks in the same moments of time, but with the patch, CPU resources are released and don't freeze at any value for a long time. It means that a fix solves the issue.
Also, I don't find any problems with common functionality - TCP/NIO transport works as expected.

@timja
Copy link
Member

timja commented Feb 28, 2022

@jeffret-b any chance you can take a look?

@jeffret-b jeffret-b added the bug For changelog: Fixes a bug. label Mar 2, 2022
@jeffret-b
Copy link
Contributor

This needs a tracking ticket in Jira. Could you please create one, @gulyaev13 ?

Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. If the result is CLOSED, there should be no reason to continue processing. The testing provided by @gulyaev13 is hopeful. At the worst, it shouldn't harm any regular situations. And it should help in some cases of a closed connection.

Approving as long as we have a Jira tracking ticket connected to it in case someone finds a problem based upon some other scenario.

@timja timja changed the title Prevent infinite loop in case of closed SSL connection JENKINS-67928 Prevent infinite loop in case of closed SSL connection Mar 2, 2022
@timja
Copy link
Member

timja commented Mar 2, 2022

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

Thanks!

@basil basil merged commit 4970956 into jenkinsci:master Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Fixes a bug. ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants