-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
xds: FileWatcherCertificateProvider is leaked #11692
Comments
@ejona86 @shivaspeaks Request you to provide the repro steps for the same so that it will be helpful for us to debug and fix the root cause/leakage issue. |
The flakiness was unrelated. Run It appears to apply to most tests, so you can choose your favorite and run just the one test. For example, in the xds/ directory, run I see |
@ejona86 @shivaspeaks As per your above suggestion I can see the below println (added in start() and close() methods ) statement for 1 of the test (as attached in below snippet) as below with 2 consecutive starts call execution followed by 1 close call. Here the expected behaviour for this UT should have only 1 start call followed by 1 close call as below? please confirm ? Executed start() |
@vinodhabib The number of start() calls should match the number of close() calls. The number of start() calls per test is likely to be 2, one for server and one for client. But this issue is that some are started but not closed. |
@ejona86 @shivaspeaks I was going through the issue and try to debug/analyze on how the close() flow is triggering for server side cert (FileWatcherCertificateProvider) release, which is part of XdsServerWrapper.java class calling I commented the below code in the shutdown() method and try to ran one of test XdsSecurityClientServerTest.tlsClientServer_noClientAuthentication which does not trigger the close() flow for cert release for server as mentioned in the below snippet As per my above analysis/observations, i feel like a similar implementation for Client ( maybe XdsClientWrapper.java) does not exist. Request you to share your inputs on my above analysis/observations also much appreciated your further hints/tips. Thanks in Advance. |
To see ClientSecurityHandler in the stack of the non-shutdown instances I did |
@ejona86 Thanks for your reply and details I will look into it. Any inputs on my above Analysis related to the similar server side implementation is missing at the client side? |
When I filed the bug I said it wasn't getting shut down, i.e., close() wasn't being called. So I don't understand what your analysis was trying to contribute. I'll note that client-side is organized quite differently than server-side. The server-side organization actually looks like a bad idea, as FilterChain doesn't have a lifetime and is part of XdsListenerResource.LdsUpdate so is multi-consumer. Yet it has SslContextProvideSupplier. But I can believe it would end up having mostly the correct user-visible behavior today. (I see at least one edge case that could leak, but it isn't that severe.) Client-side is properly not storing the supplier in CdsUpdate. And it looks like the same class closes the supplier that creates it. That's great. I also see code that appears to clean up on shutdown. |
In investigating #11678 (comment) , it was discovered that XdsSecurityClientServerTest looks to create two FileWatcherCertificateProviders each test, but only shuts down one. Since it is using the Channel/Server APIs, this means one of the client or server is highly likely leaking the certificate provider.
CC @kannanjgithub
The text was updated successfully, but these errors were encountered: