reta commented on a change in pull request #721: URL: https://github.com/apache/cxf/pull/721#discussion_r540323780
########## File path: core/src/main/java/org/apache/cxf/common/spi/ClassLoaderProxyService.java ########## @@ -0,0 +1,58 @@ +/** + * 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.cxf.common.spi; + +import java.util.Map; +import java.util.logging.Logger; + +import org.apache.cxf.Bus; +import org.apache.cxf.common.logging.LogUtils; + +public class ClassLoaderProxyService implements ClassLoaderService { + private static final Logger LOG = LogUtils.getL7dLogger(ClassLoaderProxyService.class); + NamespaceClassCreator srv; Review comment: Please declare `srv` as `private final` ########## File path: core/src/main/java/org/apache/cxf/common/spi/NamespaceClassGenerator.java ########## @@ -0,0 +1,451 @@ +/** + * 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.cxf.common.spi; + +import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; + +import org.apache.cxf.Bus; +import org.apache.cxf.common.classloader.ClassLoaderUtils; +import org.apache.cxf.common.logging.LogUtils; +import org.apache.cxf.common.util.ASMHelper; +import org.apache.cxf.common.util.OpcodesProxy; + +public class NamespaceClassGenerator extends ClassGeneratorClassLoader implements NamespaceClassCreator { + + private static final Logger LOG = LogUtils.getL7dLogger(ClassGeneratorClassLoader.class); + ASMHelper helper; Review comment: Please declare `helper` as `private final` ########## File path: core/src/main/java/org/apache/cxf/common/spi/GeneratedClassClassLoaderCapture.java ########## @@ -0,0 +1,36 @@ +/** + * 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.cxf.common.spi; + +/** Implement this interface to store class generated in order during build phase + * inject it back before runtime to avoid class generation. + * produce dot class file thanks to save method. + * You can check WrapperNamespaceClassGeneratorTest.testGeneratedFirst for usage + * Here is list of extensions to set in order to avoid class loading after generation during build time. + * bus.setExtension(new WrapperHelperClassLoader(bus), WrapperHelperCreator.class); + * bus.setExtension(new ExtensionClassLoader(bus), ExtensionClassCreator.class); + * bus.setExtension(new ExceptionClassLoader(bus), ExceptionClassCreator.class); + * bus.setExtension(new GeneratedWrapperClassLoader(bus), WrapperClassCreator.class); + * bus.setExtension(new FactoryClassLoader(bus), FactoryClassCreator.class); + * bus.setExtension(new GeneratedNamespaceClassLoader(bus), NamespaceClassCreator.class); + * @author olivier dufour + */ +public interface GeneratedClassClassLoaderCapture { + void save(String className, byte[] bytes); Review comment: Minor, may be better name for this method would be `capture`, wdyt? ########## File path: core/src/main/java/org/apache/cxf/common/spi/NamespaceClassCreator.java ########## @@ -0,0 +1,32 @@ +/** + * 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.cxf.common.spi; + +import java.util.Map; + +/** + * SPI interface to implement the proxy defining logic. + * It enables to switch from unsafe to classloader logic for instance for java >= 9. + */ +public interface NamespaceClassCreator { + + Class<?> createNamespaceWrapper(Class<?> mcls, Map<String, String> map); Review comment: We have this method `createNamespaceWrapper` here and in `ClassLoaderService`. The name is the same but those do slightly different things. May be we could rename this one to `createNamespaceWrapperClass` or the one in `ClassLoaderService` to `createNamespaceWrapperInstance`, wdy? ########## File path: rt/frontend/jaxws/src/main/java/org/apache/cxf/jaxws/spi/GeneratedWrapperClassLoader.java ########## @@ -0,0 +1,119 @@ +/** + * 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.cxf.jaxws.spi; + +import java.lang.reflect.Method; +import java.util.LinkedHashSet; +import java.util.Set; + +import org.apache.cxf.Bus; +import org.apache.cxf.common.spi.GeneratedClassClassLoader; +import org.apache.cxf.common.util.PackageUtils; +import org.apache.cxf.common.util.StringUtils; +import org.apache.cxf.jaxws.WrapperClassGenerator; +import org.apache.cxf.jaxws.support.JaxWsServiceFactoryBean; +import org.apache.cxf.service.model.InterfaceInfo; +import org.apache.cxf.service.model.MessageInfo; +import org.apache.cxf.service.model.MessagePartInfo; +import org.apache.cxf.service.model.OperationInfo; +import org.apache.cxf.wsdl.service.factory.ReflectionServiceFactoryBean; + +/** If class has been generated during build time + * (use @see org.apache.cxf.common.spi.GeneratedClassClassLoaderCapture capture to save bytes) + * you can set class loader to avoid class generation during runtime: + * bus.setExtension(new GeneratedWrapperClassLoader(bus), WrapperClassCreator.class); + * @author olivier dufour + */ +public class GeneratedWrapperClassLoader extends GeneratedClassClassLoader implements WrapperClassCreator { Review comment: Please rename to `WrapperClassLoader` to follow the convention: `ExceptionClassGenerator` / `ExceptionClassLoader`, `WrapperHelperClassLoader` / `WrapperHelperClassGenerator` ########## File path: rt/databinding/jaxb/src/main/java/org/apache/cxf/jaxb/WrapperHelperProxyService.java ########## @@ -0,0 +1,53 @@ +/** + * 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.cxf.jaxb; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; + +import org.apache.cxf.Bus; +import org.apache.cxf.databinding.WrapperHelper; + +public class WrapperHelperProxyService implements WrapperHelperCreator { + WrapperHelperCreator srv; Review comment: Please declare `srv` as `private final` ########## File path: core/src/main/java/org/apache/cxf/common/util/ASMHelper.java ########## @@ -19,447 +19,29 @@ package org.apache.cxf.common.util; -import java.lang.ref.WeakReference; -import java.lang.reflect.Constructor; -import java.lang.reflect.Field; -import java.lang.reflect.GenericArrayType; import java.lang.reflect.Method; -import java.lang.reflect.ParameterizedType; -import java.lang.reflect.TypeVariable; -import java.lang.reflect.WildcardType; -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import org.apache.cxf.common.classloader.ClassLoaderUtils; import org.apache.cxf.common.util.ReflectionInvokationHandler.Optional; import org.apache.cxf.common.util.ReflectionInvokationHandler.UnwrapParam; import org.apache.cxf.common.util.ReflectionInvokationHandler.WrapReturn; -public class ASMHelper { - protected static final Map<Class<?>, String> PRIMITIVE_MAP = new HashMap<>(); - protected static final Map<Class<?>, String> NONPRIMITIVE_MAP = new HashMap<>(); - protected static final Map<Class<?>, Integer> PRIMITIVE_ZERO_MAP = new HashMap<>(); - protected static final Map<ClassLoader, WeakReference<TypeHelperClassLoader>> LOADER_MAP - = new WeakIdentityHashMap<>(); - protected static final Map<Class<?>, WeakReference<TypeHelperClassLoader>> CLASS_MAP - = new WeakIdentityHashMap<>(); +public interface ASMHelper { + String getClassCode(Class<?> cl); + String getClassCode(java.lang.reflect.Type type); + ClassWriter createClassWriter(); + ASMType getType(String type); + Label createLabel(); + OpcodesProxy getOpCodes(); + Class<?> getASMClass() throws ClassNotFoundException; + String getMethodSignature(Method m); + String getNonPrimitive(Class<?> tp); + String getPrimitive(Class<?> tp); + void setBadASM(boolean b); Review comment: Please remove this method (`setBadASM`), it is internal impl only and is not used anywhere ########## File path: rt/bindings/corba/src/main/java/org/apache/cxf/binding/corba/utils/CorbaAnyHelper.java ########## @@ -272,258 +266,20 @@ public static void extractPrimitiveFromAny(Any a, CorbaPrimitiveHandler primitiv IDL_TO_SCHEMA_TYPES.put(CorbaConstants.NT_CORBA_ANY, W3CConstants.NT_SCHEMA_ANYTYPE); } - private static Any createFixedAny(ORB orb, Any any) { - createFixedAnyConstructor(); + private static synchronized Any createFixedAny(ORB orb, Any any) { + if (fixedAnyConstructor == null) { + CorbaFixedAnyImplGenerator corbaFixedAnyImplGenerator = new CorbaFixedAnyImplGenerator(null); Review comment: It seems like we did shortcut here by not passing the bus, could you please add `Bus` to method arguments? The `bus` instance is present in all flows, just needs to be passed around: `CorbaConduit` / `CorbaDestination` / `CorbaServerConduit` would need to be modified to accept `Bus` in constructors, and in `CorbaStreamInInterceptor` you could get the bus from `exchange` instance available in place. ########## File path: systests/jaxws/src/test/java/org/apache/cxf/systest/jaxws/ClientServerMiscTest.java ########## @@ -419,9 +420,11 @@ public void testWrappedHolderOutNull() throws Exception { } private void setASM(boolean b) throws Exception { - Field f = ASMHelper.class.getDeclaredField("badASM"); - ReflectionUtil.setAccessible(f); - f.set(null, !b); + + ASMHelper helper = getBus().getExtension(ASMHelper.class); + Method m = ASMHelper.class.getMethod("setBadASM", Boolean.TYPE); Review comment: Please use `Method m = helper.getClass().getMethod("setBadASM", Boolean.TYPE);` ########## File path: rt/frontend/jaxws/src/main/java/org/apache/cxf/jaxws/spi/WrapperClassCreatorProxyService.java ########## @@ -0,0 +1,53 @@ +/** + * 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.cxf.jaxws.spi; + +import java.util.Set; + +import org.apache.cxf.Bus; +import org.apache.cxf.jaxws.WrapperClassGenerator; +import org.apache.cxf.jaxws.support.JaxWsServiceFactoryBean; +import org.apache.cxf.service.model.InterfaceInfo; + +public class WrapperClassCreatorProxyService implements WrapperClassCreator { + WrapperClassCreator srv; Review comment: Please declare `srv` as `private final` ########## File path: rt/databinding/jaxb/src/main/java/org/apache/cxf/jaxb/FactoryClassProxyService.java ########## @@ -0,0 +1,48 @@ +/** + * 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.cxf.jaxb; + +import org.apache.cxf.Bus; + +public class FactoryClassProxyService implements FactoryClassCreator { + FactoryClassCreator srv; Review comment: Please declare `srv` as `private final` ########## File path: rt/frontend/simple/src/main/java/org/apache/cxf/endpoint/dynamic/ExceptionClassCreatorProxyService.java ########## @@ -0,0 +1,48 @@ +/** + * 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.cxf.endpoint.dynamic; + +import org.apache.cxf.Bus; + +public class ExceptionClassCreatorProxyService implements ExceptionClassCreator { + ExceptionClassCreator srv; Review comment: Please declare `srv` as `private final` ########## File path: rt/databinding/jaxb/src/main/java/org/apache/cxf/jaxb/WrapperHelperCompiler.java ########## @@ -24,78 +24,43 @@ import javax.xml.bind.JAXBElement; +import org.apache.cxf.Bus; +import org.apache.cxf.common.spi.ClassGeneratorClassLoader; import org.apache.cxf.common.util.ASMHelper; +import org.apache.cxf.common.util.OpcodesProxy; +import org.apache.cxf.common.util.StringUtils; import org.apache.cxf.databinding.WrapperHelper; -final class WrapperHelperCompiler extends ASMHelper { - - - final Class<?> wrapperType; - final Method[] setMethods; - final Method[] getMethods; - final Method[] jaxbMethods; - final Field[] fields; - final Object objectFactory; - final ClassWriter cw; - - private WrapperHelperCompiler(Class<?> wrapperType, - Method[] setMethods, - Method[] getMethods, - Method[] jaxbMethods, - Field[] fields, - Object objectFactory) { - this.wrapperType = wrapperType; - this.setMethods = setMethods; - this.getMethods = getMethods; - this.jaxbMethods = jaxbMethods; - this.fields = fields; - this.objectFactory = objectFactory; - cw = createClassWriter(); - } +public final class WrapperHelperCompiler extends ClassGeneratorClassLoader implements WrapperHelperCreator { Review comment: I think we have to rename it to `WrapperHelperClassGenerator` to be on par with `WrapperHelperClassLoader` and others: `FactoryClassGenerator` / `FactoryClassLoader`, etc. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
