[close #596] fix tls reloading encouter slow thread npe #597

iosmanthus · 2022-04-22T14:52:50Z

Signed-off-by: iosmanthus myosmanthustree@gmail.com

What problem does this PR solve?

Issue Number: close #596

Problem Description:

getChannel will encounter NPE for the slow thread might meet a null if the cert is unmodified.

What is changed and how does it work?

Add an initial state for SslContextBuilder

Check List for Tests

This PR has been tested by at least one of the following methods:

Unit test

Related changes

Need to cherry-pick the release branch
Need to update the documentation

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

codecov · 2022-04-22T15:00:45Z

Codecov Report

Merging #597 (1e077c2) into master (3163793) will increase coverage by 0.31%.
The diff coverage is 67.11%.

@@             Coverage Diff              @@
##             master     #597      +/-   ##
============================================
+ Coverage     33.82%   34.13%   +0.31%     
- Complexity     1354     1371      +17     
============================================
  Files           270      270              
  Lines         17174    17244      +70     
  Branches       1956     1958       +2     
============================================
+ Hits           5809     5887      +78     
+ Misses        10751    10746       -5     
+ Partials        614      611       -3

Impacted Files	Coverage Δ
src/main/java/org/tikv/common/ConfigUtils.java	`0.00% <ø> (ø)`
src/main/java/org/tikv/common/TiSession.java	`70.28% <0.00%> (-0.67%)`	⬇️
...va/org/tikv/common/region/StoreHealthyChecker.java	`72.15% <0.00%> (+1.63%)`	⬆️
...n/java/org/tikv/common/util/ConcreteBackOffer.java	`83.78% <0.00%> (-0.62%)`	⬇️
src/main/java/org/tikv/common/PDClient.java	`56.63% <50.00%> (-2.84%)`	⬇️
...java/org/tikv/common/operation/PDErrorHandler.java	`38.70% <50.00%> (ø)`
.../main/java/org/tikv/common/AbstractGRPCClient.java	`42.30% <61.53%> (-3.64%)`	⬇️
...main/java/org/tikv/common/util/ChannelFactory.java	`62.80% <70.40%> (+6.60%)`	⬆️
src/main/java/org/tikv/common/TiConfiguration.java	`66.86% <100.00%> (+0.87%)`	⬆️
...ain/java/org/tikv/common/util/BackOffFunction.java	`82.35% <100.00%> (+0.53%)`	⬆️
... and 12 more

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 3163793...1e077c2. Read the comment docs.

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

sunxiaoguang · 2022-04-22T22:47:32Z

src/main/java/org/tikv/common/util/ChannelFactory.java

@@ -213,14 +214,15 @@ public ChannelFactory(
    this.keepaliveTimeout = keepaliveTimeout;
    this.idleTimeout = idleTimeout;
    this.certContext = new JksContext(jksKeyPath, jksKeyPassword, jksTrustPath, jksTrustPassword);
+    reloadSslContext();
  }

  @VisibleForTesting
  public boolean reloadSslContext() {
    if (certContext != null) {
      SslContextBuilder newBuilder = certContext.reload();


There is still a race between isModified check and the rest of initialization logic.

Reload in constructor can help mitigating NPE. But the race can make later certificate reload not working as expected.

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus · 2022-04-23T09:18:15Z

@sunxiaoguang PTAL

zhangyangyu

Currently, all concurrency control applies to "getting the latest SSLContext builder" and then "use it to initialize the connection pool correctly". But what if there are two consecutive cert change happening very quickly? Then we might use two different SSLContext builder to initialize the pool since when the second reload calls connpool.clear, the first call to getChannel might still not start to initialize the pool.

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

zhangyangyu · 2022-04-26T02:41:08Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+        sslContext = sslContextBuilder.build();
+      } catch (SSLException e) {
+        logger.error("create ssl context failed!", e);
+        return null;


return null or raise exception? What should the user expect and how they could restore?

change to raise exception.

src/main/java/org/tikv/common/util/ChannelFactory.java

xuanyu66 · 2022-04-26T04:39:32Z

src/main/java/org/tikv/common/util/ChannelFactory.java

-    if (reloadSslContext()) {
-      logger.info("invalidate connection pool");
+  @VisibleForTesting
+  public synchronized ManagedChannel reload(String address, HostMapping mapping) {


I am worried about the performance of getChannel(). Since renewing cert is a very low frequent operation, it's a big cost to sync and invoke a disk IO every time

And from my point of view, client-java needs to have a high throughput when working as a cache client.
It's a situation quite like service discovery in RPC client, in which we wouldn't refresh every time getChannel

There is an optimization to avoid sync disk IO, using a WatchService to notify the ChannelFactory if the cert is changed. A TODO is added.

Would you enhance it recently?

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

…oad-slow-thread-npe

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

xuanyu66 · 2022-04-28T10:11:53Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+        p.register(watchService, StandardWatchEventKinds.ENTRY_MODIFY);
+
+        WatchKey key;
+        while ((key = watchService.take()) != null) {


Under which condition would equal null?

https://docs.oracle.com/javase/7/docs/api/java/nio/file/WatchService.html#take() should always return notnull seem this null check is redundant.

xuanyu66 · 2022-04-28T10:15:00Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+              logger.info("detected file change: {}", event.context());
+              if (event.context().toString().equals(target)) {
+                changed = true;
+                break OUTER;


Why break here?

If all the certs in the base path change in one shot, multiple events will be triggered. If we consume all of these events one by one, multiple reload will be triggered. This break controls a list of events that will trigger only once reload.

xuanyu66 · 2022-04-28T10:28:12Z

src/main/java/org/tikv/common/util/ChannelFactory.java

  private final CertContext certContext;
+
+  @VisibleForTesting
+  public final ConcurrentHashMap<Pair<SslContextBuilder, String>, ManagedChannel> connPool =


The key of Map doesn't implement hashCode() and equals(). Will it bring some potential bugs?

I will wrap SslContextBuilder into a type with epoch.

xuanyu66 · 2022-04-28T10:33:48Z

@shiyuhang0 PTAL

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus · 2022-04-28T12:55:18Z

@xuanyu66 PTAL

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

xuanyu66 · 2022-04-28T14:18:27Z

@iosmanthus Plz check the failed CI tests

xuanyu66 · 2022-04-28T14:43:56Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+          WatchKey key = watchService.take();
+          boolean changed = false;
+          OUTER:
+          for (WatchEvent<?> event : key.pollEvents()) {


key.pollEvents() is non-blocking, so the method is keeping running, which may waste CPU resource

this method will not keep running, once the events were taken out of the queue, this block will exist and wait at watchService.take()

xuanyu66 · 2022-04-28T14:50:50Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+  private void cleanExpireConn(Collection<ManagedChannel> pending) {
+    for (ManagedChannel channel : pending) {
+      logger.info("cleaning expire channels");
+      channel.shutdownNow();


Why use shutdownNow()? Maybe some channel is being used right now

If the channel is used right now, no matter calling shutdown or shutdownNow, the user of the channel will receive an exception, a retry is expected.

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

zhangyangyu · 2022-05-13T16:37:46Z

src/main/java/org/tikv/common/util/ChannelFactory.java

+      // Check all the modification of the `targets`.
+      // If one of them changed, means to need reload.
+      for (int i = 0; i < targets.size(); i++) {
+        long lastModified = targets.get(i).lastModified();


lastModified might raise error due to permission. Maybe we need to catch the whole needReload body to give a meaningful error log?

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus · 2022-05-16T13:47:57Z

@zhangyangyu @xuanyu66 @sunxiaoguang @shiyuhang0 PTAL

ti-srebot · 2022-05-17T03:42:37Z

@xuanyu66, 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.

ti-srebot · 2022-05-17T05:37:12Z

@shiyuhang0, 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.

ti-srebot · 2022-05-17T13:46:15Z

@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.

sunxiaoguang · 2022-04-29T03:15:47Z

src/main/java/org/tikv/common/util/ChannelFactory.java


+  private int connRecycleTime;


This should be final as well.

sunxiaoguang · 2022-05-17T14:54:02Z

/merge

ti-srebot · 2022-05-17T14:54:36Z

/run-all-tests

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus · 2022-05-17T15:10:09Z

/run-all-tests

sunxiaoguang · 2022-05-17T15:17:20Z

/run-all-tests

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

fix tls reloading encouter slow thread npe

c9dd664

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus added 2 commits April 22, 2022 23:01

add tests for mulithread cert reload

57603a5

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

./dev/javafmt

bf6f7fa

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

sunxiaoguang reviewed Apr 22, 2022

View reviewed changes

iosmanthus added 2 commits April 23, 2022 16:54

refactor with rwlock

ce12f28

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

./dev/javafmt

baa9bd8

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

zhangyangyu reviewed Apr 23, 2022

View reviewed changes

refactor channel management

619efab

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

zhangyangyu reviewed Apr 26, 2022

View reviewed changes

src/main/java/org/tikv/common/util/ChannelFactory.java Outdated Show resolved Hide resolved

xuanyu66 reviewed Apr 26, 2022

View reviewed changes

iosmanthus added 3 commits April 26, 2022 15:11

raise exception while encouter ssl exception

d8593b8

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

Merge branch 'master' of github.com:tikv/client-java into fix-tls-rel…

9460f91

…oad-slow-thread-npe

prevent channel leak while updating cert context

0d16453

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

github-actions bot added the component/common label Apr 27, 2022

iosmanthus added 3 commits April 27, 2022 19:41

fix ha issue for getMembers and checkHealth

46da216

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

fix mock pd tests

2b0050d

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

./dev/javafmt

5f032cc

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

xuanyu66 suggested changes Apr 28, 2022

View reviewed changes

address comments

8e9c66f

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

./dev/javafmt

ca97cef

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

xuanyu66 reviewed Apr 28, 2022

View reviewed changes

iosmanthus added 2 commits April 28, 2022 23:34

fix wrong initial state of ChannelFactory

63d631c

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

./dev/javafmt

7a64cda

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

zhangyangyu reviewed May 13, 2022

View reviewed changes

iosmanthus added 2 commits May 15, 2022 23:44

git fire: fix wrong certReloadInterval settings for OpenSslContext

2097d39

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

polish logs and comments

9a71def

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

xuanyu66 previously approved these changes May 17, 2022

View reviewed changes

shiyuhang0 previously approved these changes May 17, 2022

View reviewed changes

zhangyangyu previously approved these changes May 17, 2022

View reviewed changes

sunxiaoguang previously approved these changes May 17, 2022

View reviewed changes

ti-srebot added the status/LGT1 label May 17, 2022

ti-srebot added the status/can-merge label May 17, 2022

make connRecycleTime final

548f243

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus dismissed stale reviews from sunxiaoguang, zhangyangyu, shiyuhang0, and xuanyu66 via 548f243 May 17, 2022 15:00

sunxiaoguang previously approved these changes May 17, 2022

View reviewed changes

fix unstable tests

cef6195

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus dismissed sunxiaoguang’s stale review via cef6195 May 17, 2022 15:44

./dev/javafmt

1e077c2

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>

iosmanthus enabled auto-merge (squash) May 17, 2022 15:54

sunxiaoguang approved these changes May 18, 2022

View reviewed changes

iosmanthus merged commit 774ee50 into tikv:master May 18, 2022

Daemonxiao mentioned this pull request Jun 1, 2022

Add TLS reload pingcap/tispark#2365

Merged

[close #596] fix tls reloading encouter slow thread npe #597

[close #596] fix tls reloading encouter slow thread npe #597

Conversation

iosmanthus commented Apr 22, 2022

What problem does this PR solve?

What is changed and how does it work?

Check List for Tests

Related changes

codecov bot commented Apr 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iosmanthus commented Apr 23, 2022

zhangyangyu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iosmanthus Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuanyu66 commented Apr 28, 2022

iosmanthus commented Apr 28, 2022

xuanyu66 commented Apr 28, 2022

xuanyu66 Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iosmanthus commented May 16, 2022

ti-srebot commented May 17, 2022

ti-srebot commented May 17, 2022

ti-srebot commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunxiaoguang commented May 17, 2022

ti-srebot commented May 17, 2022

iosmanthus commented May 17, 2022

sunxiaoguang commented May 17, 2022

codecov bot commented Apr 22, 2022 •

edited

Loading

zhangyangyu left a comment •

edited

Loading

iosmanthus Apr 26, 2022 •

edited

Loading

xuanyu66 Apr 28, 2022 •

edited

Loading