[Functions] Fix classloader leaks

[lhotari]

Motivation

While investigating error code 143 problems in DataStax Pulsar Helm Chart CI, I noticed that some classloaders are leaked.

Modifications

• Fix classloader leak in FunctionCommon.getClassLoaderFromPackage
• Fix classloader leak in SinksImpl and SourcesImpl

Documentation

• no-need-doc

[nicoloboschi]

LGTM

[jerrypeng]

@lhotari the lines that actually changed in pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java

doesn't seem as much as git is displaying. Is there formatting issue? It is hard to see which lines actually changed.

[lhotari]

@lhotari the lines that actually changed in pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionCommon.java

doesn't seem as much as git is displaying. Is there formatting issue? It is hard to see which lines actually changed.

@jerrypeng I don't seem to have that issue in the GitHub Web UI. The diff shows the changes. Which diff tool are you using?

[jerrypeng]

@lhotari I am just using the github Web UI. In FunctionCommon.java is it all that you added was a try / finally that wraps the the existing code in getClassLoaderFromPackage method?

[lhotari]

@lhotari I am just using the github Web UI. In FunctionCommon.java is it all that you added was a try / finally that wraps the the existing code in getClassLoaderFromPackage method?

@jerrypeng Now I see what you mean. Yes the diff is pretty hard to read. Yes, I wrapped with another try catch where the finally block is

} finally {
    if (!keepJarClassLoader) {
        closeClassLoader(jarClassLoader);
    }
    if (!keepNarClassLoader) {
        closeClassLoader(narClassLoader);
    }
}

[lhotari]

besides the finally block, I added the lines to set keepNarClassLoader and keepJarClassLoader as needed.

[jerrypeng]

@lhotari this LGTM but can you add some unit tests for this?

[lhotari]

@lhotari this LGTM but can you add some unit tests for this?

@jerrypeng Thanks for reviewing. I can add tests, but the barrier for that is that it feels hard to unit test classloader leaks. Can you recommend an approach for this particular case?

[eolivelli]

LGTM

adding tests is hard in this case.
I am not sure about how to do it.

hooking into Class loading is hard and also it will probably depend on the JVM/Java version.

We could count the calls to "ClassLoaderUtils.closeClassLoader" using PowerMock, but I am not sure it is worth