Github user jgallimore commented on a diff in the pull request:
https://github.com/apache/tomee/pull/332#discussion_r244586465
--- Diff:
container/openejb-core/src/main/java/org/apache/openejb/OpenEjbContainer.java
---
@@ -71,57 +50,74 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
-import java.util.Set;
+import java.util.*;
import java.util.logging.LogManager;
import static org.apache.openejb.cdi.ScopeHelper.startContexts;
import static org.apache.openejb.cdi.ScopeHelper.stopContexts;
/**
- * @version $Rev$ $Date$
+ * Embeddable {@link EJBContainer} implementation based
+ * on OpenEJB container.
*/
public final class OpenEjbContainer extends EJBContainer {
+
static {
// if tomee embedded was ran we'll lost log otherwise
- final String logManger =
JavaSecurityManagers.getSystemProperty("java.util.logging.manager");
- if (logManger != null) {
+ final String logManager =
JavaSecurityManagers.getSystemProperty("java.util.logging.manager");
+ if (logManager != null) {
try {
-
Thread.currentThread().getContextClassLoader().loadClass(logManger);
+
Thread.currentThread().getContextClassLoader().loadClass(logManager);
} catch (final Exception ignored) {
- final Field field;
+ Field field = null;
+ boolean accessible = false;
try {
field = LogManager.class.getDeclaredField("manager");
- field.setAccessible(true);
- field.set(null, new
JuliLogStreamFactory.OpenEJBLogManager());
+ if(field != null){
+ accessible = field.isAccessible();
+ field.setAccessible(true);
+ field.set(null, new
JuliLogStreamFactory.OpenEJBLogManager());
+ }
+
} catch (final Exception ignore) {
// ignore
+ }finally{
+ if(field != null){
+ field.setAccessible(accessible);
--- End diff --
@gerdogdu There's a couple of comments for you on the dev@ mailing list:
Is there any particular reason for flipping the imports to wildcards? We
tend to import the individual classes elsewhere in the codebase. My personal
preference would be to leave those as individual imports. It doesn't make the
import list that much longer, and I find its clearer what is being imported and
used. IDEs tend to fold those away unless you're specifically looking at them
too.
IntelliJ is definitely preferring the Java 8 style
`moduleLocations.removeIf`, and will refactor the loop to it with one
keystroke. I have the what is probably an unpopular opinion which is that I
actually prefer the loop over the lambda. The reasons are:
1. Backporting changes is much harder when you have to work around language
changes like this
2. I personally find lots of things crammed into one line harder to read,
and in this particular case, there's a cast and a negation (so a double
negative) in the removeIf.
I am currently running tests against your PR and master, along with code
coverage. Although I appreciate you have extracted methods to make this more
readable and not changed functionality, it is a core class, and I think running
those checks is a reasonable thing to do.
Generally speaking, I think this looks ok, and if you can reply to those
couple of points on the list, I think we can get this merged in.
Thanks again for taking the time to create the PR.
---
