You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/11/25 07:44:59 UTC
[GitHub] [pulsar] lhotari opened a new pull request #12973: [Functions] Fix classloader leaks
lhotari opened a new pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973
### 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
- [x] `no-need-doc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985269470
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985268427
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985271166
> @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);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985283101
> @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 test classloader leaks. Can you recommend an approach for this particular case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#discussion_r756729129
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourcesImpl.java
##########
@@ -728,20 +729,28 @@ public SourceConfig getSourceInfo(final String tenant,
}
}
- // if source is not builtin, attempt to extract classloader from package file if it exists
- if (classLoader == null && sourcePackageFile != null) {
- classLoader = getClassLoaderFromPackage(sourceConfig.getClassName(),
- sourcePackageFile, worker().getWorkerConfig().getNarExtractionDirectory());
- }
+ boolean shouldCloseClassLoader = false;
+ try {
+ // if source is not builtin, attempt to extract classloader from package file if it exists
+ shouldCloseClassLoader = true;
Review comment:
thanks for catching that @nicoloboschi . I pushed a fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985272261
besides the finally block, I added the lines to set `keepNarClassLoader` and `keepJarClassLoader` as needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on a change in pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#discussion_r756684661
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourcesImpl.java
##########
@@ -728,20 +729,28 @@ public SourceConfig getSourceInfo(final String tenant,
}
}
- // if source is not builtin, attempt to extract classloader from package file if it exists
- if (classLoader == null && sourcePackageFile != null) {
- classLoader = getClassLoaderFromPackage(sourceConfig.getClassName(),
- sourcePackageFile, worker().getWorkerConfig().getNarExtractionDirectory());
- }
+ boolean shouldCloseClassLoader = false;
+ try {
+ // if source is not builtin, attempt to extract classloader from package file if it exists
+ shouldCloseClassLoader = true;
Review comment:
here the `ClassLoader` must be closed only if `classLoader` is null, otherwise you will close the `ClassLoader` of the `Connector` instance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari edited a comment on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985283101
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985069805
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973#issuecomment-985268427
> @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.
I don't seem to have that issue in the GitHub Web UI. The diff shows the changes. Which diff tool are you using?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #12973: [Functions] Fix classloader leaks
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #12973:
URL: https://github.com/apache/pulsar/pull/12973
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org