Skip to content
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

[fix][broker] EntryFilters fix NoClassDefFoundError due to closed classloader #22767

Merged
merged 5 commits into from
May 29, 2024

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented May 23, 2024

Motivation

When unloading a topic the EntryFilterWithClassLoader instance closes the NarClassLoader that loaded the filter, preventing any further loading of classes.

This is a regression introduced in 3.0.0 by #19364

Reproducer:

  • configure one EntryFilter in broker.conf
  • create a topic (this in turn creates an EntryFilterWithClassLoader instance)
  • do not use the EntryFilter (don't consume messages)
  • unload the topic (this calls EntryFilterWithClassLoader.close() that calls ClassLoader.close())
  • consume data from any topic that uses the entry filter
  • CRASH ! because the Classloader is closed and it cannot load any other classes

Sample Stacktrace (this is from the DataStax fork, but it shows the problem):

2024-05-09T13:16:13,145+0000 [broker-topic-workers-OrderedExecutor-0-0] ERROR org.apache.bookkeeper.common.util.SingleThreadExecutor - Error while running task: com/datastax/oss/pulsar/jms/selectors/SelectorSupport
java.lang.NoClassDefFoundError: com/datastax/oss/pulsar/jms/selectors/SelectorSupport
        at com.datastax.oss.pulsar.jms.selectors.JMSFilter.parseSelector(JMSFilter.java:309) ~[?:?]
        at com.datastax.oss.pulsar.jms.selectors.JMSFilter.filterEntry(JMSFilter.java:245) ~[?:?]
        at com.datastax.oss.pulsar.jms.selectors.JMSFilter.filterEntry(JMSFilter.java:130) ~[?:?]
        at org.apache.pulsar.broker.service.plugin.EntryFilterWithClassLoader.filterEntry(EntryFilterWithClassLoader.java:41) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.EntryFilterSupport.getFilterResult(EntryFilterSupport.java:85) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.EntryFilterSupport.runFiltersForEntry(EntryFilterSupport.java:66) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.AbstractBaseDispatcher.filterEntriesForConsumer(AbstractBaseDispatcher.java:151) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.trySendMessagesToConsumers(PersistentDispatcherMultipleConsumers.java:749) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.sendMessagesToConsumers(PersistentDispatcherMultipleConsumers.java:654) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.lambda$readEntriesComplete$8(PersistentDispatcherMultipleConsumers.java:619) ~[com.datastax.oss-pulsar-broker-3.1.4.1-SNAPSHOT.jar:3.1.4.1-SNAPSHOT]
        at org.apache.bookkeeper.common.util.SingleThreadExecutor.safeRunTask(SingleThreadExecutor.java:137) ~[org.apache.bookkeeper-bookkeeper-common-4.16.5xxxx]
        at org.apache.bookkeeper.common.util.SingleThreadExecutor.run(SingleThreadExecutor.java:113) ~[org.apache.bookkeeper-bookkeeper-common-4.16.5xxxx]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[io.netty-netty-common-4.1.108.Final.jar:4.1.108.Final]
        at java.lang.Thread.run(Thread.java:842) ~[?:?]
Caused by: java.lang.ClassNotFoundException: com.datastax.oss.pulsar.jms.selectors.SelectorSupport
        at java.net.URLClassLoader.findClass(URLClassLoader.java:445) ~[?:?]
        at java.lang.ClassLoader.loadClass(ClassLoader.java:587) ~[?:?]
        at java.lang.ClassLoader.loadClass(ClassLoader.java:520) ~[?:?]
        ... 14 more

Modifications

Mofications:

  • make it clear that EntryFilterWithClassLoader is not owning the classloader
  • enforce also using the Thread ContextClassloader in EntryFilterWithClassLoader (this is another problem, but it didn't affect the specific usecase reported by the user)

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: eolivelli#29

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 23, 2024
@eolivelli eolivelli self-assigned this May 23, 2024
@eolivelli eolivelli added type/bug The PR fixed a bug or issue reported a bug area/broker ready-to-test labels May 23, 2024
@eolivelli eolivelli added this to the 3.4.0 milestone May 23, 2024
@eolivelli eolivelli changed the title [broker] EntryFilters fix NoClassDefFoundError due to closed classloader [fix][broker] EntryFilters fix NoClassDefFoundError due to closed classloader May 23, 2024
@eolivelli eolivelli requested a review from lhotari May 23, 2024 09:13
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Good work @eolivelli

@lhotari
Copy link
Member

lhotari commented May 23, 2024

@eolivelli test failure

 org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'EntryFilterWithClassLoader'
  	at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgsRecordingInvocations(BrokerTestUtil.java:61)
  	at org.apache.pulsar.broker.service.plugin.FilterEntryTest.testFilter(FilterEntryTest.java:242)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
  	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
  	at org.testng.internal.invokers.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:47)
  	at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:76)
  	at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
  	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
  	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
  	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
  	at java.base/java.lang.Thread.run(Thread.java:840)
  Caused by: org.mockito.creation.instance.InstantiationException: 
  Unable to create instance of 'EntryFilterWithClassLoader'.
  Please ensure that the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.service.plugin.EntryFilterTest, org.apache.pulsar.common.nar.NarClassLoader].

dao-jun
dao-jun previously approved these changes May 24, 2024
@eolivelli
Copy link
Contributor Author

@dao-jun I have updated the code and removed ClassLoaderSwitcher

@dao-jun dao-jun merged commit 5a7efd8 into apache:master May 29, 2024
49 of 50 checks passed
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request May 31, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 4, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 4, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants