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

[close #619] Shutdown recycler when closing ChannelFactory to avoid resource leak #618

Merged
merged 6 commits into from
Jun 22, 2022

Conversation

Daemonxiao
Copy link
Contributor

@Daemonxiao Daemonxiao commented Jun 20, 2022

Signed-off-by: Daemonxiao [email protected]

What problem does this PR solve?

Issue Number: close #619

Because the recycler in ChannelFactory does not set any task when TLS is turned off, the process will get stuck on exit.

What is changed and how does it work?

When we don't use TLS, recycler should be null.

Code changes

  • Has exported function/method change

Side effects

  • NO side effects

Related changes

  • Need to cherry-pick to the release branch

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #618 (7742a6d) into master (d6a15c4) will decrease coverage by 0.05%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #618      +/-   ##
============================================
- Coverage     34.72%   34.67%   -0.06%     
+ Complexity     1425     1423       -2     
============================================
  Files           278      278              
  Lines         17354    17356       +2     
  Branches       1972     1972              
============================================
- Hits           6027     6018       -9     
- Misses        10713    10735      +22     
+ Partials        614      603      -11     
Impacted Files Coverage Δ
...main/java/org/tikv/common/util/ChannelFactory.java 62.64% <83.33%> (+0.43%) ⬆️
...va/org/tikv/common/region/StoreHealthyChecker.java 69.62% <0.00%> (-3.80%) ⬇️
...rc/main/java/io/grpc/netty/NettyClientHandler.java 57.11% <0.00%> (-0.87%) ⬇️
...ty/handler/codec/http2/Http2ConnectionHandler.java 51.10% <0.00%> (-0.49%) ⬇️
src/main/java/io/grpc/internal/ClientCallImpl.java 56.58% <0.00%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6a15c4...7742a6d. Read the comment docs.

@iosmanthus
Copy link
Member

@Daemonxiao Could you create an issue with this pull request? The PR title checker will check if the PR is related to an issue.

@Daemonxiao Daemonxiao changed the title Fixed a hang bug when TLS is turned off [close #619] Fixed a hang bug when TLS is turned off Jun 20, 2022
Signed-off-by: Daemonxiao <[email protected]>
Co-authored-by: iosmanthus <[email protected]>
@iosmanthus
Copy link
Member

The tests for ChannelFactory might leak executors because some certWatcher or ChannelFactory are not closed automatically with the try-with-resource block. Could you fix it within this pull request? I think these two issues are related. Rest LGTM

@Daemonxiao
Copy link
Contributor Author

/hold

@Daemonxiao Daemonxiao changed the title [close #619] Fixed a hang bug when TLS is turned off Shutdown recycler when closing ChannelFactory to avoid resource leak Jun 21, 2022
@ti-srebot
Copy link
Collaborator

@zhangyangyu, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. You are not a reviewer or committer or co-leader or leader.

@Daemonxiao Daemonxiao changed the title Shutdown recycler when closing ChannelFactory to avoid resource leak [close #619]Shutdown recycler when closing ChannelFactory to avoid resource leak Jun 21, 2022
@Daemonxiao Daemonxiao changed the title [close #619]Shutdown recycler when closing ChannelFactory to avoid resource leak [close #619] Shutdown recycler when closing ChannelFactory to avoid resource leak Jun 21, 2022
Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

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

LGTM

@iosmanthus iosmanthus merged commit 3724e87 into tikv:master Jun 22, 2022
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Jun 22, 2022
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.3 in PR #621

iosmanthus pushed a commit that referenced this pull request Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScheduledExecutorService may not be shutdown in ChannelFactory
4 participants