[rfc] [icedtea-web] singletons logic, logs and test cleanup/fixes
Jiri Vanek
jvanek at redhat.com
Tue Dec 17 07:29:10 PST 2013
hi!
Motivation - after "console aware of plugin" patch, some tests started *sometimes* die as "can not
intialize x11" or "null" (not NPE, but just "null" it was that test was null - so class was not even
statically instantiated as I found later).
Imho the "console aware of plugin" was not the cause, but make this error more visible.
Unfortunately i have not found the exact cause, but made few changes which make logic more direct
and from those (five) I think two were the cause of "null" and "x11" errors.
The rewritten console pathc on review have to be adapted to this.
fix 1: initialisation of console gui done in time when gui is shown, not when console is first time
"used" - this is crucial for "x11" error.
fix2: the NoStdOutErrTest.java methods do not throw any Exception now. Instead of this they log it
and pretend to be ok. This was crucial for "null" error. Sometimes race condition or "x11" error (or
null was causing x111 ???) throw exception here, which confused junit bloody much. a) it was
consumed b)test was not working
fix3: removed internal MessageWithLevel class. It lost it sense with addition of headers.
fix4: debug messages from plugin are not going directly to console output, but are buffered in
already exisiting messageQue. I'm not sure how good idea it is. It can lower the synchronisation issues.
fix5: all static factory methods made synchronised. Imho it is the only correct, but.... :) Also I
made the synchronisation via "synchronised" but I'm not sure how correct it is... Maybe via static
final Object lock = new Object(); is better?
Comments of my own inline:
> diff -r ebb3f3d96b55 netx/net/sourceforge/jnlp/util/logging/JavaConsole.java
> --- a/netx/net/sourceforge/jnlp/util/logging/JavaConsole.java Mon Dec 16 13:18:08 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/util/logging/JavaConsole.java Mon Dec 16 18:57:47 2013 +0100
> @@ -62,15 +62,12 @@
> import javax.swing.JSplitPane;
> import javax.swing.JTextArea;
> import javax.swing.SwingUtilities;
> -import javax.swing.UIManager;
> import javax.swing.border.EmptyBorder;
> import javax.swing.border.TitledBorder;
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> import net.sourceforge.jnlp.util.ImageResources;
> import net.sourceforge.jnlp.util.logging.headers.Header;
> -import net.sourceforge.jnlp.util.logging.headers.JavaMessage;
> -import net.sourceforge.jnlp.util.logging.headers.MessageWithHeader;
> import net.sourceforge.jnlp.util.logging.headers.PluginMessage;
>
> /**
> @@ -86,7 +83,7 @@
> private static JavaConsole console;
> private static Dimension lastSize;
>
> - public static JavaConsole getConsole() {
> + public synchronized static JavaConsole getConsole() {
> if (console == null) {
> console = new JavaConsole();
> }
synchronised factory method
> @@ -119,15 +116,22 @@
> private JDialog consoleWindow;
> private JTextArea stdErrText;
> private JTextArea stdOutText;
> - private JPanel contentPanel = new JPanel();
> + private JPanel contentPanel;
> private ClassLoaderInfoProvider classLoaderInfoProvider;
> + private boolean initialized = false;
> +
> + private String stdErrTextSrc = "";
> + private String stdOutTextSrc = "";
The logging is now done to strings, and those strings are populated to textfields only if console
was ever visible (initialized)
>
> public JavaConsole() {
> - initialize();
> +
> }
no gui initialisationnow.
>
>
> private void initializeWindow() {
> + if (!initialized){
> + initialize();
> + }
> initializeWindow(lastSize, contentPanel);
> }
gui initialise with window.
>
> @@ -160,13 +164,6 @@
> */
> private void initialize() {
>
> - try {
> - UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
> - } catch (Exception e) {
> - OutputController.getLogger().log(OutputController.Level.ERROR_ALL, e);
> - }
> -
> -
IS this really necessary code?
> contentPanel = new JPanel();
> contentPanel.setLayout(new GridBagLayout());
>
> @@ -296,6 +293,7 @@
>
> splitPane.setDividerLocation(0.5);
> splitPane.setResizeWeight(0.5);
> + initialized = true;
> }
after gui is created.
>
> public void showConsole() {
> @@ -416,22 +414,21 @@
> }
>
>
> - void addMessage(Header header, String message) {
> - if (!LogConfig.getLogConfig().isEnableHeaders()){
> + synchronized void addMessage(Header header, String message) {
> + String headerString = "";
> + if (LogConfig.getLogConfig().isEnableHeaders()){
> + headerString = header.toString();
> + }
> if (header.level.isError()){
> - stdErrText.setText(stdErrText.getText() + message + "\n");
> + stdErrTextSrc += headerString + message +"\n";
> }
> if (header.level.isOutput()){
> - stdOutText.setText(stdOutText.getText() + message + "\n");
> + stdOutTextSrc += headerString + message + "\n";
> }
> - } else {
> - if (header.level.isError()){
> - stdErrText.setText(stdErrText.getText( )+ header.toString() + message +"\n");
> + if (initialized){
> + stdErrText.setText(stdErrTextSrc);
> + stdOutText.setText(stdOutTextSrc);
> }
> - if (header.level.isOutput()){
> - stdOutText.setText(stdOutText.getText() + header.toString() + message + "\n");
> - }
> - }
> }
This method is more simple now. and is "logging" into strings. Those are refresing textareas only
if initialised.
>
> /**
> @@ -440,7 +437,7 @@
> */
> private void processPluginMessage(String s) {
> PluginMessage pm = new PluginMessage(s);
> - addMessage(pm.getHeader(), pm.getMessage());
> + OutputController.getLogger().log(pm);
> }
This is the only line valid for fix4.
>
>
> diff -r ebb3f3d96b55 netx/net/sourceforge/jnlp/util/logging/LogConfig.java
> --- a/netx/net/sourceforge/jnlp/util/logging/LogConfig.java Mon Dec 16 13:18:08 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/util/logging/LogConfig.java Mon Dec 16 18:57:47 2013 +0100
> @@ -37,8 +37,6 @@
> package net.sourceforge.jnlp.util.logging;
>
> import java.io.File;
> -import javax.naming.ConfigurationException;
> -
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
>
> @@ -58,7 +56,7 @@
>
> private static LogConfig logConfig;
>
> - public LogConfig() {
> + private LogConfig() {
logcongif is created via factory methods only.
> DeploymentConfiguration config = JNLPRuntime.getConfiguration();
> // Check whether logging and tracing is enabled.
> enableLogging =
Boolean.parseBoolean(config.getProperty(DeploymentConfiguration.KEY_ENABLE_LOGGING));
> @@ -81,7 +79,7 @@
> }
> }
>
> - public static LogConfig getLogConfig() {
> + public synchronized static LogConfig getLogConfig() {
> if (logConfig == null) {
> logConfig = new LogConfig();
> }
> @@ -89,7 +87,7 @@
> }
>
> /** For testing only: throw away the previous config */
> - static void resetLogConfig() {
> + static synchronized void resetLogConfig() {
> if (logConfig != null) {
> logConfig = new LogConfig();
> }
two factory methods made synchronised
> diff -r ebb3f3d96b55 netx/net/sourceforge/jnlp/util/logging/OutputController.java
> --- a/netx/net/sourceforge/jnlp/util/logging/OutputController.java Mon Dec 16 13:18:08 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/util/logging/OutputController.java Mon Dec 16 18:57:47 2013 +0100
> @@ -38,10 +38,12 @@
> import java.io.PrintStream;
> import java.io.PrintWriter;
> import java.io.StringWriter;
> -import java.util.Date;
> import java.util.LinkedList;
> import java.util.List;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.util.logging.headers.Header;
> +import net.sourceforge.jnlp.util.logging.headers.JavaMessage;
> +import net.sourceforge.jnlp.util.logging.headers.MessageWithHeader;
>
> public class OutputController {
>
> @@ -88,19 +90,6 @@
> }
> }
>
> - private static final class MessageWithLevel {
> -
> - public final String message;
> - public final Level level;
> - public final StackTraceElement[] stack = Thread.currentThread().getStackTrace();
> - public final Thread thread = Thread.currentThread();
> - public final Date loggedAt = new Date();
> -
> - public MessageWithLevel(String message, Level level) {
> - this.message = message;
> - this.level = level;
> - }
> - }
removed inner class, repalced by MessageWithHeader class hierarchy.
> /*
> * singleton instance
> */
> @@ -110,7 +99,7 @@
> private PrintStreamLogger outLog;
> private PrintStreamLogger errLog;
> private SingleStreamLogger sysLog;
> - private List<MessageWithLevel> messageQue = new LinkedList<MessageWithLevel>();
> + private List<MessageWithHeader> messageQue = new LinkedList<MessageWithHeader>();
Adapted to new interface
> private MessageQueConsumer messageQueConsumer = new MessageQueConsumer();
>
> //bounded to instance
> @@ -149,33 +138,32 @@
> }
>
> private void consume() {
> - MessageWithLevel s = messageQue.get(0);
> + MessageWithHeader s = messageQue.get(0);
> messageQue.remove(0);
> - net.sourceforge.jnlp.util.logging.headers.Header header = new
net.sourceforge.jnlp.util.logging.headers.Header(s.level, s.stack, s.thread, s.loggedAt, false);
> //filtering is done in console during runtime
> if (LogConfig.getLogConfig().isLogToConsole()) {
> - JavaConsole.getConsole().addMessage(header, s.message);
> + JavaConsole.getConsole().addMessage(s.getHeader(), s.getMessage());
> }
> - if (!JNLPRuntime.isDebug() && (s.level == Level.MESSAGE_DEBUG
> - || s.level == Level.WARNING_DEBUG
> - || s.level == Level.ERROR_DEBUG)) {
> + if (!JNLPRuntime.isDebug() && (s.getHeader().level == Level.MESSAGE_DEBUG
> + || s.getHeader().level == Level.WARNING_DEBUG
> + || s.getHeader().level == Level.ERROR_DEBUG)) {
> //filter out debug messages
> //must be here to prevent deadlock, casued by exception form jnlpruntime, loggers or
configs themselves
> return;
> }
> - String message = s.message;
> + String message = s.getMessage();
> if (LogConfig.getLogConfig().isEnableHeaders()) {
> if (message.contains("\n")) {
> - message = header.toString() + "\n" + message;
> + message = s.getHeader().toString() + "\n" + message;
> } else {
> - message = header.toString() + " " + message;
> + message = s.getHeader().toString() + " " + message;
> }
> }
> if (LogConfig.getLogConfig().isLogToStreams()) {
> - if (s.level.isOutput()) {
> + if (s.getHeader().level.isOutput()) {
> outLog.log(message);
> }
> - if (s.level.isError()) {
> + if (s.getHeader().level.isError()) {
> errLog.log(message);
> }
only adaptation of MessageWithLevel -> MessageWithHeader change
> }
> @@ -225,9 +213,7 @@
> Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
> @Override
> public void run() {
> - while (!messageQue.isEmpty()) {
> - consume();
> - }
> + flush();
> }
> }));
> }
> @@ -303,24 +289,29 @@
> }
>
> private synchronized void log(Level level, Object o) {
> + String s ="";
> if (o == null) {
> - messageQue.add(new MessageWithLevel(NULL_OBJECT, level));
> + s = NULL_OBJECT;
> } else if (o instanceof Throwable) {
> - messageQue.add(new MessageWithLevel(exceptionToString((Throwable) o), level));
> + s = exceptionToString((Throwable) o);
> } else {
> - messageQue.add(new MessageWithLevel(o.toString(), level));
> + s=o.toString();
> }
> + log(new JavaMessage(new Header(level, false), s));
> + }
> +
> + synchronized void log(MessageWithHeader l){
> + messageQue.add(l);
> this.notifyAll();
> }
only adaptation of MessageWithLevel -> MessageWithHeader change
> -
> - private FileLog getFileLog() {
> + private synchronized FileLog getFileLog() {
> if (fileLog == null) {
> fileLog = new FileLog();
> }
> return fileLog;
> }
>
> - private SingleStreamLogger getSystemLog() {
> + private synchronized SingleStreamLogger getSystemLog() {
> if (sysLog == null) {
> if (JNLPRuntime.isWindows()) {
two factory methods made synchronised
> sysLog = new WinSystemLog();
> diff -r ebb3f3d96b55 netx/net/sourceforge/jnlp/util/logging/headers/Header.java
> --- a/netx/net/sourceforge/jnlp/util/logging/headers/Header.java Mon Dec 16 13:18:08 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/util/logging/headers/Header.java Mon Dec 16 18:57:47 2013 +0100
> @@ -42,8 +42,9 @@
> import net.sourceforge.jnlp.util.logging.OutputController.Level;
>
> public class Header {
> -
> - public String user;
> + public static String default_user = System.getProperty("user.name");
> +
> + public String user = default_user;
> public boolean application;
> public Level level;
> public Date date = new Date();
> @@ -56,12 +57,15 @@
> public Header() {
> }
>
> + public Header(Level level, boolean isC) {
> + this(level, Thread.currentThread().getStackTrace(), Thread.currentThread(), isC);
> + }
this information were previously fetched from MessageWithLevel
> +
> public Header(Level level, StackTraceElement[] stack, Thread thread, boolean isC) {
> this(level, stack, thread, new Date(), isC);
> }
>
> public Header(Level level, StackTraceElement[] stack, Thread thread, Date d, boolean isC) {
> - this.user = System.getProperty("user.name");
Call in constructor caused deadlock (tahts why moved to static initialisation)
> this.application = JNLPRuntime.isWebstartApplication();
> this.level = level;
> this.date = d;
> @@ -146,7 +150,8 @@
> result = stack[i];//at least moving up
> if (stack[i].getClassName().contains(OutputController.class.getName())
> || //PluginDebug.class.getName() not avaiable during netx make
> - stack[i].getClassName().contains("sun.applet.PluginDebug")) {
> + stack[i].getClassName().contains("sun.applet.PluginDebug")
> + || stack[i].getClassName().contains(Header.class.getName())) {
> continue;
> } else {
> break;
> diff -r ebb3f3d96b55 netx/net/sourceforge/jnlp/util/logging/headers/PluginHeader.java
> --- a/netx/net/sourceforge/jnlp/util/logging/headers/PluginHeader.java Mon Dec 16 13:18:08 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/util/logging/headers/PluginHeader.java Mon Dec 16 18:57:47 2013 +0100
> @@ -62,15 +62,6 @@
> }
>
> @Override
> - public String toString(boolean userb, boolean originb, boolean levelb, boolean dateb,
boolean callerb, boolean thread1b, boolean thread2b) {
> - if (preinit) {
> - return "!" + super.toString(userb, originb, levelb, dateb, callerb, thread1b, thread2b);
> - } else {
> - return super.toString(userb, originb, levelb, dateb, callerb, thread1b, thread2b);
> - }
> - }
This was actuallybug. It was adding two "!" instead of one "!". The override is kept.
> -
> - @Override
> public String thread1ToString() {
> return " ITNPP Thread# " + thread1;
> }
> diff -r ebb3f3d96b55 tests/netx/unit/net/sourceforge/jnlp/util/logging/JavaConsoleTest.java
> --- a/tests/netx/unit/net/sourceforge/jnlp/util/logging/JavaConsoleTest.java Mon Dec 16 13:18:08
2013 +0100
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/logging/JavaConsoleTest.java Mon Dec 16 18:57:47
2013 +0100
> @@ -21,7 +21,6 @@
> @Test
> public void CreatePluginHeaderTestOK() throws Exception{
> PluginMessage p1 = new PluginMessage(s1);
> - System.out.println(p1.header + p1.restOfMessage);
this was bug missed during review.
> PluginMessage p3 = new PluginMessage(s3);
> Assert.assertFalse(p1.wasError);
> Assert.assertFalse(p3.wasError);
> diff -r ebb3f3d96b55 tests/test-extensions/net/sourceforge/jnlp/util/logging/NoStdOutErrTest.java
> --- a/tests/test-extensions/net/sourceforge/jnlp/util/logging/NoStdOutErrTest.java Mon Dec 16
13:18:08 2013 +0100
> +++ b/tests/test-extensions/net/sourceforge/jnlp/util/logging/NoStdOutErrTest.java Mon Dec 16
18:57:47 2013 +0100
catched and logged exceptions in this class.
> @@ -34,11 +34,11 @@
> obligated to do so. If you do not wish to do so, delete this
> exception statement from your version.
> */
> +
> package net.sourceforge.jnlp.util.logging;
>
> -import java.lang.reflect.Field;
> -import java.lang.reflect.InvocationTargetException;
> import java.lang.reflect.Method;
> +import net.sourceforge.jnlp.ServerAccess;
> import org.junit.AfterClass;
> import org.junit.BeforeClass;
>
> @@ -54,36 +54,47 @@
> * by junit classlaoder is visible from itw, but not vice verse.
> */
> public class NoStdOutErrTest {
> -
> +
> private static boolean origialStds;
> -
> private static final String setLogToStreams = "setLogToStreams";
> -
> - @BeforeClass
> - public static void disableStds() throws Exception {
> - //init logger and log and flush message
> - //it is crucial for junit to grip it
> - OutputController.getLogger().log("initialising");
> - //one more times: if TESTED class is the first which creates instance of logger
> - //then when junit can not access this class, and creates its own for its purposes
> - //when junit creates this class, then also TESTED class have access to it and so it
behaves as expected
> - OutputController.getLogger().flush();
> - origialStds = LogConfig.getLogConfig().isLogToStreams();
> - invokeSetLogToStreams(false);
> - }
> -
> - @AfterClass
> - public static void restoreStds() throws Exception {
> - OutputController.getLogger().flush();
> - invokeSetLogToStreams(origialStds);
> + /*
> + * "printed" exceptions are otherwise consumed via junit if thrown :-/
> + */
> +
> + @BeforeClass
> + public static synchronized void disableStds() {
> + try {
> + //init logger and log and flush message
> + //it is crucial for junit to grip it
> + OutputController.getLogger().log("initialising");
> + //one more times: if TESTED class is the first which creates instance of logger
> + //then when junit can not access this class, and creates its own for its purposes
> + //when junit creates this class, then also TESTED class have access to it and so it
behaves as expected
> + OutputController.getLogger().flush();
> + origialStds = LogConfig.getLogConfig().isLogToStreams();
> + invokeSetLogToStreams(false);
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + }
> }
>
> - private static void invokeSetLogToStreams(boolean state) throws IllegalAccessException,
SecurityException, NoSuchMethodException, InvocationTargetException, IllegalArgumentException {
> - Method lcs = LogConfig.class.getDeclaredMethod(setLogToStreams, boolean.class);
> - lcs.setAccessible(true);
> - lcs.invoke(LogConfig.getLogConfig(), state);
> + @AfterClass
> + public static synchronized void restoreStds() {
> + try {
> + OutputController.getLogger().flush();
> + invokeSetLogToStreams(origialStds);
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + }
> }
> -
> -
> -
> +
> + private static synchronized void invokeSetLogToStreams(boolean state) {
> + try {
> + Method lcs = LogConfig.class.getDeclaredMethod(setLogToStreams, boolean.class);
> + lcs.setAccessible(true);
> + lcs.invoke(LogConfig.getLogConfig(), state);
> + } catch (Exception ex) {
> + ServerAccess.logException(ex);
> + }
> + }
> }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixedNpeInUnittTests.patch
Type: text/x-patch
Size: 17674 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131217/cb3300ed/fixedNpeInUnittTests-0001.patch
More information about the distro-pkg-dev
mailing list