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

[java] Add close method to JDK 11 client. Ensure close methods for Http client is called. #11345

Merged
merged 2 commits into from
Dec 3, 2022

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Dec 1, 2022

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #11270.

Motivation and Context

  1. The memory leak is prominently seen when Augmenter is used which in turn creates a WebSocket connection using the HTTP Client to create a CDP connection. Though webdriver implementation supporting CDP does the same. The same client instance needs to be closed to ensure no resources are being held when the driver quits. The changes add the required code to achieve this.

  2. Additionally, we supply an executor to the JDK 11 Http client for the worker threads, which is shutdown when the client is closed along with the underlying websocket. We also ensure there is no reference to the JDK 11 Http client instance after closing the client. This has helped fix the ever-increasing threads related to the JDK 11 Http client. HttpClientImpl (Java 11's implementation) does a timely check internally to see if no references to the client are there then closes the Selector Manager threads which are later collected when the GC kicks in.

The combination of the above 2 changes has helped ensure that there is no leak from JDK 11 Http client in different scenarios.

Before the changes:
Screenshot 2022-12-01 at 12 58 39 PM

After the changes:
Screenshot 2022-11-30 at 8 18 37 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@diemol
Copy link
Member

diemol commented Dec 1, 2022

We need to get the CI passing first though.

@pujagani
Copy link
Contributor Author

pujagani commented Dec 2, 2022

Thank you for helping review. +1 , need to ensure CI tests run and it is all green.

@sonarcloud
Copy link

sonarcloud bot commented Dec 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
23.5% 23.5% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants