JetBrains
[AA REx] Require a parent Disposable when creating resolve extensions.
Per the documented API for Resolve Extensions, the KaResolveExtension instance is Disposable, and will be cleaned up by the creator when it is no longer necessary. This is handled by LLFirSession for the extensions created for analysis, but the extensions created by KaResolveExtensionToContentScopeRefinerBridge did not dispose their created extensions, causing resource leaks in REx clients.
To fix this, and to enforce proper handling of the Disposable lifecycle for KaResolveExtension users, require passing a Disposable factory into provideExtensionsFor, which will be used as a parent disposable for any created extensions. For efficiency, the factory will not be called if no extensions are created.
@marcopennekamp PTAL - found a bug in Resolve Extensions after 25.2 merge.
@blueshiftlabs Hey, great find, and thanks for the fix!
Can you please create a YouTrack issue and link it to the commit?
Also, we now have a place where we create a resolve extension on the fly. This makes me think: It would be pretty nice if resolve extensions themselves didn't have a lifecycle.
One big issue on the LL FIR side is that we have to support a lot of complicated machinery to allow LL FIR sessions to have a Disposable. We have a custom map implementation, CleanableValueReferenceCache, which has to invoke the LLFirSessionCleaner to dispose the disposable even when the session is garbage collected. Without that disposable, we could use an off-the-shelf concurrent map and only call the session cleaner for actively invalidated sessions. The need to invalidate the disposable after garbage collection is the main issue.
It's in general pretty bad design to rely on an action happening after garbage collection. Disposables should ideally be disposed deterministically, and not rely on such volatile parents as LL FIR sessions (which can be weakly referenced at any time).
Some issues which are caused by this complexity:
- https://youtrack.jetbrains.com/issue/KT-75018/Analysis-API-Flaky-failures-in-CleanableWeakValueReferenceCacheLincheckTest
- Concurrent tests are difficult to investigate, even with Lincheck.
- I cannot link the issue, but we have rare cases where a session in the cache is already marked invalid, which should not be possible, and is related to the complex cache implementation.
- And of course, issues like the bug you fix in this review can only happen because the resolve extension has a complicated lifecycle. One which, in my opinion, does not weigh up to the scope of the feature.
Do you think we could move the logic which requires the lifecycle to be moved outside the resolve extension?
Can you perhaps link me all the usages of the resolve extension as a disposable on your side so that I can take a closer look? Maybe we can come up with a cleaner design here.
