apache / incubator-pegasus

Apache Pegasus - A horizontally scalable, strongly consistent and high-performance key-value store
https://pegasus.apache.org/
Apache License 2.0
1.96k stars 310 forks source link

fix(java-client): fix client renew TGT thread leak when request table doesnt exist #1970

Closed Samunroyu closed 2 months ago

Samunroyu commented 2 months ago

In the Kerberos environment, java client will check TGT and relogin kerberos if needed every 10 seconds by a thread. But when client requests table which doesnt exist will cause threads leak.

The root cause this problem is when Pegasus client close the client but don't close TGT thread pool.

This patch adds KerberosProtocol close function.

shalk commented 2 months ago

Handle Exception is a good choice. Exception will make the schedule job not schedule again

But i am not quite understand the main cause about daemon
I don't kown the relatetionship between daemon thread with exception handler @Samunroyu

if you set daemon false, you should shutdown the threadpool when client jvm shutdown.

Apache9 commented 2 months ago

Handle Exception is a good choice. Exception will make the schedule job not schedule again

But i am not quite understand the main cause about daemon I don't kown the relatetionship between daemon thread with exception handler @Samunroyu

if you set daemon false, you should shutdown the threadpool when client jvm shutdown.

I‘ve talked with @Samunroyu offline, the root cause here is we do not have a close method for KerberosProtocol class, so if we create the client many time due to kerberos authentication error, although we have already closed all the created client, the KerberosProtocol will not be closed, so the ExecutorService will be leaked and cause too many threads.

shalk commented 2 months ago

Handle Exception is a good choice. Exception will make the schedule job not schedule again But i am not quite understand the main cause about daemon I don't kown the relatetionship between daemon thread with exception handler @Samunroyu if you set daemon false, you should shutdown the threadpool when client jvm shutdown.

I‘ve talked with @Samunroyu offline, the root cause here is we do not have a close method for KerberosProtocol class, so if we create the client many time due to kerberos authentication error, although we have already closed all the created client, the KerberosProtocol will not be closed, so the ExecutorService will be leaked and cause too many threads.

It make sense. I get the case about thread leak.

if the pr finish and ci pass, click request , i will review again @Samunroyu