ramanathan1504 commented on code in PR #4363: URL: https://github.com/apache/logging-log4j2/pull/4363#discussion_r4129912915
########## src/changelog/.2.x.x/4339_jndi_manager_context_isolation.xml: ########## @@ -0,0 +1,12 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns="https://logging.apache.org/xml/ns"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation=" + https://logging.apache.org/xml/ns + https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="fixed"> + <issue id="4339" link="https://github.com/apache/logging-log4j2/issues/4339"/> Review Comment: ```suggestion <issue id="4339" link="https://github.com/apache/logging-log4j2/issues/4339"/> <issue id="4363" link="https://github.com/apache/logging-log4j2/pull/4363"/> ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/net/JndiManager.java: ########## @@ -159,11 +157,20 @@ public static JndiManager getJndiManager( * @since 2.9 */ public static JndiManager getJndiManager(final Properties properties) { - return getManager(createManagerName(), FACTORY, properties); + return createManager(JndiManager.class.getName(), properties); } - private static String createManagerName() { - return JndiManager.class.getName() + '@' + JndiManager.class.hashCode(); + private static JndiManager createManager(final String name, final Properties properties) { + if (!isJndiEnabled()) { + throw new IllegalStateException( + String.format("JNDI must be enabled by setting one of the %s* properties to true", PREFIX)); + } + try { + return new JndiManager(name, new InitialContext(properties)); + } catch (final NamingException e) { + LOGGER.error("Error creating JNDI InitialContext for '{}'.", name, e); + throw new IllegalStateException("Unable to create JNDI InitialContext for '" + name + "'", e); Review Comment: Replacing this with `return null` keeps every test green. Can we cover it with a factory that throws? ```java public static final class FailingInitialContextFactory implements InitialContextFactory { @Override public Context getInitialContext(final Hashtable<?, ?> environment) throws NamingException { throw new NamingException("test"); } } @Test void testNamingExceptionIsRethrown() { System.setProperty("log4j2.enableJndiJms", TRUE); try { final Properties properties = new Properties(); properties.setProperty(Context.INITIAL_CONTEXT_FACTORY, FailingInitialContextFactory.class.getName()); assertThrows(IllegalStateException.class, () -> JndiManager.getJndiManager(properties)); } finally { System.clearProperty("log4j2.enableJndiJms"); } } ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/net/JndiManager.java: ########## @@ -225,7 +232,7 @@ public static Properties createProperties( } @Override - protected boolean releaseSub(final long timeout, final TimeUnit timeUnit) { + public boolean stop(final long timeout, final TimeUnit timeUnit) { Review Comment: Deleting this override keeps every test green, and the context is then never closed. Can a test check the mock `Context` gets `close()`? ```java private static final List<Context> CONTEXTS = new ArrayList<>(); final Context context = mock(Context.class); CONTEXTS.add(context); return context; @Test void testCloseClosesContext() throws Exception { System.setProperty("log4j2.enableJndiJms", TRUE); CONTEXTS.clear(); try { final Properties properties = new Properties(); properties.setProperty(Context.INITIAL_CONTEXT_FACTORY, TestInitialContextFactory.class.getName()); JndiManager.getJndiManager(properties).close(); verify(CONTEXTS.get(0)).close(); } finally { System.clearProperty("log4j2.enableJndiJms"); } } ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
