From ae4bd522f364f77dbc9688e0ce902f7c61611adb Mon Sep 17 00:00:00 2001 From: Qiang Zhao <74767115+mattisonchao@users.noreply.github.com> Date: Tue, 4 Jan 2022 22:34:35 +0800 Subject: [PATCH] Change ContextClassLoader to NarClassLoader in AdditionalServlet (#13501) ### Motivation It's ``AddtionalServlet`` side change, like #11270 ### Modifications Change context class loader through Thread.currentThread().setContextClassLoader(classLoader) before every protocol handler callback, and change it back to original class loader afterwards. --- .../pulsar/broker/ClassLoaderSwitcher.java | 37 ++++++ .../AdditionalServletWithClassLoader.java | 18 ++- .../ProtocolHandlerWithClassLoader.java | 19 +-- .../AdditionalServletWithClassLoaderTest.java | 114 ++++++++++++++++++ 4 files changed, 166 insertions(+), 22 deletions(-) create mode 100644 pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ClassLoaderSwitcher.java create mode 100644 pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ClassLoaderSwitcher.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ClassLoaderSwitcher.java new file mode 100644 index 00000000000000..787182ef012bca --- /dev/null +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ClassLoaderSwitcher.java @@ -0,0 +1,37 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker; + +/** + * Help to switch the class loader of current thread to the NarClassLoader, and change it back when it's done. + * With the help of try-with-resources statement, the code would be cleaner than using try finally every time. + */ +public class ClassLoaderSwitcher implements AutoCloseable { + private final ClassLoader prevClassLoader; + + public ClassLoaderSwitcher(ClassLoader classLoader) { + prevClassLoader = Thread.currentThread().getContextClassLoader(); + Thread.currentThread().setContextClassLoader(classLoader); + } + + @Override + public void close() { + Thread.currentThread().setContextClassLoader(prevClassLoader); + } +} \ No newline at end of file diff --git a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java index 06a51c856c75dd..7f1af734431ef6 100644 --- a/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java +++ b/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoader.java @@ -19,9 +19,11 @@ package org.apache.pulsar.broker.web.plugin.servlet; import java.io.IOException; + import lombok.Data; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.apache.pulsar.broker.ClassLoaderSwitcher; import org.apache.pulsar.common.configuration.PulsarConfiguration; import org.apache.pulsar.common.nar.NarClassLoader; import org.eclipse.jetty.servlet.ServletHolder; @@ -39,22 +41,30 @@ public class AdditionalServletWithClassLoader implements AdditionalServlet { @Override public void loadConfig(PulsarConfiguration pulsarConfiguration) { - servlet.loadConfig(pulsarConfiguration); + try (ClassLoaderSwitcher ignored = new ClassLoaderSwitcher(classLoader)) { + servlet.loadConfig(pulsarConfiguration); + } } @Override public String getBasePath() { - return servlet.getBasePath(); + try (ClassLoaderSwitcher ignored = new ClassLoaderSwitcher(classLoader)) { + return servlet.getBasePath(); + } } @Override public ServletHolder getServletHolder() { - return servlet.getServletHolder(); + try (ClassLoaderSwitcher ignored = new ClassLoaderSwitcher(classLoader)) { + return servlet.getServletHolder(); + } } @Override public void close() { - servlet.close(); + try (ClassLoaderSwitcher ignored = new ClassLoaderSwitcher(classLoader)) { + servlet.close(); + } try { classLoader.close(); } catch (IOException e) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/protocol/ProtocolHandlerWithClassLoader.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/protocol/ProtocolHandlerWithClassLoader.java index 223cf81ccdb560..63aa6696917d91 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/protocol/ProtocolHandlerWithClassLoader.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/protocol/ProtocolHandlerWithClassLoader.java @@ -26,6 +26,7 @@ import lombok.Data; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.apache.pulsar.broker.ClassLoaderSwitcher; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.service.BrokerService; import org.apache.pulsar.common.nar.NarClassLoader; @@ -95,22 +96,4 @@ public void close() { log.warn("Failed to close the protocol handler class loader", e); } } - - /** - * Help to switch the class loader of current thread to the NarClassLoader, and change it back when it's done. - * With the help of try-with-resources statement, the code would be cleaner than using try finally every time. - */ - private static class ClassLoaderSwitcher implements AutoCloseable { - private final ClassLoader prevClassLoader; - - ClassLoaderSwitcher(ClassLoader classLoader) { - prevClassLoader = Thread.currentThread().getContextClassLoader(); - Thread.currentThread().setContextClassLoader(classLoader); - } - - @Override - public void close() { - Thread.currentThread().setContextClassLoader(prevClassLoader); - } - } } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java new file mode 100644 index 00000000000000..c875e8ac129cf8 --- /dev/null +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/web/plugin/servlet/AdditionalServletWithClassLoaderTest.java @@ -0,0 +1,114 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.web.plugin.servlet; + +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNull; +import org.apache.pulsar.broker.ServiceConfiguration; +import org.apache.pulsar.common.configuration.PulsarConfiguration; +import org.apache.pulsar.common.nar.NarClassLoader; +import org.eclipse.jetty.servlet.ServletHolder; +import org.testng.annotations.Test; + + +/** + * Unit test {@link AdditionalServletWithClassLoader}. + */ +@Test(groups = "broker") +public class AdditionalServletWithClassLoaderTest { + + @Test + public void testWrapper() { + AdditionalServlet servlet = mock(AdditionalServlet.class); + NarClassLoader loader = mock(NarClassLoader.class); + AdditionalServletWithClassLoader wrapper = new AdditionalServletWithClassLoader(servlet, loader); + // test getBasePath + String basePath = "bathPath"; + when(servlet.getBasePath()).thenReturn(basePath); + assertEquals(basePath, wrapper.getBasePath()); + verify(servlet, times(1)).getBasePath(); + // test loadConfig + ServiceConfiguration conf = new ServiceConfiguration(); + wrapper.loadConfig(conf); + verify(servlet, times(1)).loadConfig(same(conf)); + // test getServlet + assertEquals(wrapper.getServlet(),servlet); + // test getServletHolder + ServletHolder servletHolder = new ServletHolder(); + when(servlet.getServletHolder()).thenReturn(servletHolder); + assertEquals(wrapper.getServletHolder(),servletHolder); + verify(servlet, times(1)).getServletHolder(); + } + + @Test + public void testClassLoaderSwitcher() throws Exception { + NarClassLoader narLoader = mock(NarClassLoader.class); + AdditionalServlet servlet = new AdditionalServlet() { + @Override + public void loadConfig(PulsarConfiguration pulsarConfiguration) { + assertEquals(Thread.currentThread().getContextClassLoader(), narLoader); + } + + @Override + public String getBasePath() { + assertEquals(Thread.currentThread().getContextClassLoader(), narLoader); + return "base-path"; + } + + @Override + public ServletHolder getServletHolder() { + assertEquals(Thread.currentThread().getContextClassLoader(), narLoader); + return null; + } + + @Override + public void close() { + assertEquals(Thread.currentThread().getContextClassLoader(), narLoader); + } + }; + + AdditionalServletWithClassLoader additionalServletWithClassLoader = + new AdditionalServletWithClassLoader(servlet, narLoader); + ClassLoader curClassLoader = Thread.currentThread().getContextClassLoader(); + // test class loader + assertEquals(additionalServletWithClassLoader.getClassLoader(), narLoader); + // test getBasePath + assertEquals(additionalServletWithClassLoader.getBasePath(), "base-path"); + assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader); + // test loadConfig + ServiceConfiguration conf = new ServiceConfiguration(); + additionalServletWithClassLoader.loadConfig(conf); + assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader); + // test getServletHolder + assertNull(additionalServletWithClassLoader.getServletHolder()); + assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader); + // test getServlet + assertEquals(additionalServletWithClassLoader.getServlet(), servlet); + assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader); + // test close + additionalServletWithClassLoader.close(); + assertEquals(Thread.currentThread().getContextClassLoader(), curClassLoader); + + } +}