[rfc][icedtea-web] pre-review - accidental logging tweeking
Jacob Wisor
gitne at gmx.de
Fri Oct 16 14:52:03 UTC 2015
On 10/14/2015 at 03:20 PM Jiri Vanek wrote:
> On 10/14/2015 12:38 PM, Jacob Wisor wrote:
>> On 10/13/2015 at 06:15 PM Jiri Vanek wrote:
>>> Hello!
>>>
>>> This (small!!) patch is doing several things. I will post them separately but as
>>> about 4x4 ways is leading to destination, I wont to share thoughts before
>>> fulfilling targets of this multi-patch
>>>
>>> It started by one bug report and one future request.
>>> 1 - the bug is, that if set logging to files, and put blocked destination as
>>> ustom log file (eg "/") whole logging crashes, as initializer of FileLog keeps
>>> crashing with classNotFound.
>>> 2 - the feature request was add file-logging of applications run inside in
>>> itw. I agreed on that that as it gave quite an sense and was supposed to be
>>> easy. INstead it reviled one fragile area in logging.
>>>
>>> Two issues I found during implementation of those two above
>>> 3 - when verbose is on - junittest may deadlock
>>> - it went even more wrong during original impelmentations of implementation
>>> of 1
>>> 4 - for some already forgotten reason we are using java.util.logging as main
>>> logging facility for filelogging.
>>> - imho it is overcomplicated - several synchronizations on unexpeted palces
>>> and overall complication, where bufferedwritter.write(string) may be enough.
>>> - as side effect, we may get deadlock anytime application is using custom
>>> implementation of logger - I faced several on Elluminate. And again it went
>>> worse (here read: deadlock was more probable)
>>
>> Before we proceed with any options, I would really urge you to get rid of all
>> hardcoded thread
>> sleeps for synchronization. There is no need for them and the time slices
>> you've chosen are
>
> They are not there. There is really only few sleeps in itw, and none for
> synchronization:
>
> ./netx/net/sourceforge/jnlp/security/ConnectionFactory.java:
> Thread.sleep(100);
> - used in endless lop when connection is forced to run single-threaded
> - single-threaded, so should be ok,definitely not suspicious from any
> deadlocking
> ./netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/BasePainter.java:
> Thread.sleep(MOOVING_TEXT_DELAY);
> ./netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/BasePainter.java:
> Thread.sleep((waterLevel / 4) * 30);
> ./netx/net/sourceforge/jnlp/splashscreen/impls/defaultsplashscreen2012/ErrorPainter.java:
> Thread.sleep(FLYING_ERROR_DELAY
> - making the animation slow. Any Idea how to make it better? Againg, no
> synchronization.
> ./netx/net/sourceforge/jnlp/util/logging/OutputController.java: Thread.sleep(100);
> - this is the new one from this patch - Again - the workers around this
> thread are not intentionally not synchronized
I was talking about sleeps in logging only.
So why did you put in then?
> The rest, including:
> ./netx/net/sourceforge/jnlp/util/logging/ConsoleOutputPaneModel.java:
> Thread.sleep(new Random().nextInt(2000));
> Are in tests only and have sense. If not then any fixing of them is more
> expensive then any ManHours ITW have.
>
>
>> absolutely arbirary, and thus may or may not work on any target machine. So,
>> this is number 1.
>> Number 2, these sleeps are most probably also the root cause of any deadlocks.
>> If you start relying
>
> Disagree and disagree. Sorry.
>
>> on arbitrary thread sleeps for synchronization, this is almost always a sign
>> of bad design. So, we
>> should look at your design first before we get to any implementation options.
>
> Agree, but you will have to pin point more and probably also suggest solution.
>>
>>> So - the first is bug, and should be fixed for both head and 1.6 I fixed it by
>>> moving initializxations to factory method and returning dummy SimpleLogger
>>> doing nothing if exception occures. Its this part:
>>>
>>>
>>> + public static SingleStreamLogger createFileLog(FileLogType t) {
>>> + SingleStreamLogger s;
>>> + try {
>>> + s = new FileLog(t);
>>> + } catch (Exception ex) {
>>> + //we do not wont to block whole logging just because initialization
>>> error
>>> + OutputController.getLogger().log(ex);
>>> + s = new SingleStreamLogger() {
>>> +
>>> + @Override
>>> + public void log(String s) {
>>> + //dummy
>>> + }
>>> +
>>> + @Override
>>> + public void close() {
>>> + //dummy
>>> + }
>>> + };
>>> + }
>>> + return s;
>>> + }
>>>
>>> Part of it was refactoring in OutputController where FileLogHolder have to keep
>>> interface of SingleStreamLogger ratehr then impelmentation of FileLog and
>>> implying override of close().
>>>
>>>
>>>
>>> When I promissed (2), I thought that I will just create
>>> + private static class getAppFileLog {
>>> +
>>> + //https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java
>>> + //https://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom
>>> + private static volatile SingleStreamLogger INSTANCE =
>>> FileLog.createFileLog(FileLog.FileLogType.CLIENTAPP);
>>> + }
>>> +
>>> + public SingleStreamLogger getAppFileLog() {
>>> + return getAppFileLog.INSTANCE;
>>> + }
>>>
>>> next to already existing getFileLog and FileLogHolder.
>>>
>>> and on place of
>>> //clients app's messages are reprinted only to console
>>> if (s.getHeader().isClientApp){
>>> return;
>>> }
>>> I will jsut log one more line.
>>>
>>> However, I found that any modifications here leads to immidate unexpeted and
>>> rpetty serious deadlock - during various unittest,inruntime and so on... I
>>> solved them by adding second
>>> queue and second consume. Well I' was not happy from that... But if (4) will not
>>> be faced, it will remain the only option for this approach.
>>>
>>> When I screwed implementation of (2) so much, that the deadlock in Elluinate was
>>> 100% I tried to debug it, but as I ended in case that it is theirs custom Logger
>>> implementation which is the root cause. So I tried to rework FileLog so it do
>>> not rely in java.util.logging,but on simple FileWriter.
>>> Astonishingly it solved both (3) and (4) - when only (4) was target.
>>>
>>>
>>> For (2) there is one more approach, of which I'm not 100% sure what impact it
>>> may have. That approach is to get rid of teeOutptuStream's logic.
>>> Just for reminder - itw is taking stdout/err and is repalcing them by
>>> TeeoutputStreem which a) send what it get to original stream and (as second end
>>> of Tee) it logs this utput to our console.
>>> My idea is, to get rid of this duplcate-streams logic, and only log the stuff
>>> our client aplication is printing, And when que of logged messages is read,
>>> print it to stdout/err as originall application originally desired. Benefit will
>>> be, that this logging exeption will be rid of, but drawback will be possile
>>> lagging of messages.
>>> If this approach will be taken, then I think the seond queue+conslumer will not
>>> be needed for (2)
>>>
>>>
>>> Now - I would like to fix (1) - i think the patch is ok, I will send it
>>> separately, and (1) will be fixed for head and 1.6. Mybe push it as it is after
>>> some silence on this patch.
>>>
>>> Then, I would like to implement (2) and fix (4) ideally also (3)
>>> To do so, I need - a)get rid of teeOutputStream or/and b)getRid of our misuse
>>> of java.util.Logging and/or/maybeNot implement the second queue (its based on
>>> what of a) and/or b) will be seelcted.
>>>
>>> As I do not know any other cure for (3) then (b), I'm voting for following my
>>> patch in this email, and to repalce java.util.logging by FileWriter - we do not
>>> need whole Logging chain in ITW - afaic ...
>>>
>>>
>>> I) If (b) will be agreed on first , then implementation of (2) should be
>>> revisited and I will again check what is more suitable - whether change of
>>> TeeStreem or new Queue+consumer (maybe none? but that I doubt a bit)
>>>
>>> II) if removal of TeeStream in favour of logging and reprinting custom client
>>> apps outputs willbe agreed then (b) should be revisited if needed for (2) - but
>>> as it is the only cure for (3) and issue with (4) is just more hodden, thenI'm
>>> really for (I)
>>
>> It's nice that you start thinking but you should probably break up your
>> patches and give them simple
>
> I will and I worte it in header of this email "...ing several things. I will
> post them separately but as about 4x4 ways ..."
>
> I needed possible reviewer to understand my course, and to suggest which way he
> prefffere, so approach may be discussed before actual code is written.
> Otherwise discussion deadlocks as it do so often.
Well, obviously you're not following your own intentions since you've already
pushed your patch. Now it is obvious to me that double standards are apply here.
We /were/ discussing and you've just pushed.
>> and sane titles, denoting what purpose they are supposed to serve, for better
>> reading. Because you
>> are struggling with the English language to make yourself clear, you should
>> start simple, instead of
>> conditionally hypothesizing.
>
> Well, yes .. and no. Yes for struggling with english to trying to explain my
> thouts. And no for start simple. The upcoming patches will be simple, but Once
> the direction is agreed.
>
> Anyway I think I already made my mind in the various steps.
Yeah, that's the obvious problem now, isn't it? So, then what do you need review
for? You're still always doing stuff your way.
> Basically, I would like to
> - push the fix for initialization error
> - remake the file log to use writer instead of logging
> - I think I will keep possible swithc to original java.util.logging approach
> - move the logs implementation to autocloeable - thanx for agreeing - I was
> deeply thinking about it uring this patch, but came to conclusion it is worthy
> of separate change-set.
> - keeping in the Tee output stream. stdout of application should be as
> strightforward as possible. not wrapped and malformed by inner logging.
> - revisit the client-app file logs.
>
>>
>>> diff -r 2682417d5671 netx/net/sourceforge/jnlp/util/logging/FileLog.java
>>> --- a/netx/net/sourceforge/jnlp/util/logging/FileLog.java Thu Oct 08
>>> 11:52:14 2015 +0200
>>> +++ b/netx/net/sourceforge/jnlp/util/logging/FileLog.java Tue Oct 13
>>> 17:23:54 2015 +0200
>>> @@ -36,8 +36,11 @@
>>> exception statement from your version. */
>>> package net.sourceforge.jnlp.util.logging;
>>>
>>> +import java.io.BufferedWriter;
>>> import java.io.File;
>>> +import java.io.FileOutputStream;
>>> import java.io.IOException;
>>> +import java.io.OutputStreamWriter;
>>> import java.text.SimpleDateFormat;
>>> import java.util.Date;
>>> import java.util.logging.FileHandler;
>>> @@ -55,41 +58,80 @@
>>> private static SimpleDateFormat fileLogNameFormatter = new
>>> SimpleDateFormat("yyyy-MM-dd_HH:mm:ss.S");
>>> /**"Tue Nov 19 09:43:50 CET 2013"*/
>>> private static SimpleDateFormat pluginSharedFormatter = new
>>> SimpleDateFormat("EEE MMM dd
>>> HH:mm:ss ZZZ yyyy");
>>
>> These date formats need some clean up. If I am assuming correctly then
>> fileLogNameFormatter formats
>
> Why? This format was chosen as it is equally easily achievable from C part and
> from Java part. Also, unlike many others the output is same for C and Java part.
This is mainly due to the fact that machines in large corporate networks are
spread over multiple timezones. It's 2015, and well past the dawn of the
internet. So, having to deal with software still focused on one timezone only,
gives admins headaches (to say the least). Having consistent reliable log file
names based on ISO 8601 in UTC would enable automated log aggregation and
unified timezone independent debugging. This is also why, today, all simple text
file logging is like reinventing the wheel since all major operating systems
(yes, even Android does) feature system logging services with message time
stamping to UTC and remote aggregation. So, no sane person /should/ be using
simple text log files anymore. Nevertheless, I understand that for some people
simple text log files may still pose a nice feature. But, it should still follow
best practices learned from system logging services, like time stamping messages
in UTC and following standards for interoperability.
The date and time format you have chosen is based purely on your gut feeling,
despite the fact that an ISO standard does exist, which is especially well
suited for machine parsing. To me, doing otherwise without any objective reason
is just pure ignorance, arrogance, or laziness. You can produce ISO 8601
formatted date and time in C as easy as in Java, and vice verse. So, your
argument does not apply.
>> the name of the log file. So far so good but since the log file name is locale
>> (and timezone)
>> agnostic or rather should be, the date and time format should follow the ISO
>> format here. This would
>
> And thats correct. They should be like this. They are user;s logs, and he knws
> when he lunched itw, so its easy for him to find corresponding log.
No, he does not! Who cares when he or she launches a program? Do you always take
a look at your watch before you launch every program? And, what about admins
sitting a dozen timezones away, trying to figure out what is going on? Do you
really think, it's a lot of fun to figure out what happened when, while hundreds
of machines in three different timezones start making trouble? Yeah, good luck
with that!
> Moving them
> to utc or out of his timezone have nos ense. TWhy the logs should ne in
> different timezone then?!!?
It's probably best if I refer from here on to the discussions syslog, journald,
and Windows Event Log developers had... Man, just start looking outside of your
small world for once.
>> also allow for safe and consistent automated enumeration of log files.
>> Unfortunately, only since
>> Java 8 there is a simple way to get ISO 8601 formatted date and time. So, you
>> may want to consider
>> using this SimpleDateFormat for Java 7 and earlier:
>>
>> new SimpleDateFormat("YYYY-MM-dd'T'HH:mm:ss.S'Z'")
>>
>> And this is how you convert the local time into UTC (in principle):
>>
>> final Date date;
>> new SimpleDateFormat("YYYY-MM-dd'T'HH:mm:ss.S'Z'").format(new Date((date =
>> Date()).getTime() -
>> TimeZone.getDefault().getOffset(date.getTime())))
>>
>> There is another way; by setting the user.timezone property to UTC before
>> formatting, and then
>> setting it back to the initial timezone. But, this is really not the preferred
>> way to do it since
>> other threads may be depending on the value of the user.timezone property at
>> the same time.
>>
>> Please remember that there is literally *never* any need to do custom
>> formatting of date and time,
>> except to get ISO 8601 formatting in Java 7 and earlier. Always use either ISO
>> or the locale
>> specific formatting. Java is pretty good at that.
>
> All above have valid answer in my previous paagraph - both about timezone and
> about format.
Nothing you said is valid because it is not based on facts. Just pure gut feeling.
>> Furthermore, I am assuming that pluginSharedFormatter is the formatter used
>> for time stamping
>> messages written into the log file. This may not be locale agnostic anymore
>> because it is read by
>> human beings. So, this should probably be DateFormat.getDateTimeInstance().
>> Or, again a ISO 8601
>> formatter, if automated parsing is required.
>>
>> Besides, fileLogNameFormatter and pluginSharedFormatter should probably be
>> both final. Again,
>
> ok.
>> fileLogNameFormatter and pluginSharedFormatter are terrible names because they
>> actually do not give
>> a clear clue to what they are for.
>
> Suggestion? Imho those are good names...
logFileNameFormatter
and
appletLogFileNameFormatter or jnlpAppLogFileNameFormatter, as I assume this one
is for formatting the name of the applet's or JNLP application's log file name.
But, of course, I may be wrong because if something is "plug-in shared" then you
may expect to find some output from the native plug-in world in it.
>>> + private static final String defaultloggerName = "IcedTea-Web file-logger";
>>> + private static final String defaultApploggerName = "IcedTea-Web
>>> app-file-logger";
>>
>> Don't we have a constant for the package/product name, which should be rather
>> used for this purpose,
>> somewhere?
>
> Right! We have and ashes to my head!
Good, I was not aware either, but this is what I would have looked for first, in
this case.
>>> + private final BufferedWriter bw;
>>>
>>> - private final Logger impl;
>>
>>> - private final FileHandler fh;
>>> - private static final String defaultloggerName = "IcedTea-Web file-logger";
>>> + private static String getPrefixById(FileLogType id) {
>>> + switch (id) {
>>> + case NETX:
>>> + return "itw-javantx-";
>>> + case CLIENTAPP:
>>> + return "itw-clienta-";
>>>
>>> - public FileLog() {
>>> - this(false);
>>> - }
>>> -
>>> - public FileLog(boolean append) {
>>> - this(defaultloggerName, LogConfig.getLogConfig().getIcedteaLogDir()
>>> + "itw-javantx-" +
>>> getStamp() + ".log", append);
>>> + }
>>
>> Fix brace formatting, please.
>
> I will skip formatting issues now, as all the patches will be posted in smaller
> hunks, whose should be well formated. (ty anyway!)
>>
>>> + throw new RuntimeException("Unknow id " + id);
>>> }
>>
>> Fix "Unknow" to "Unknown", please.
>>
>>> + private static String getNameById(FileLogType id) {
>>> + switch (id) {
>>> + case NETX:
>>> + return defaultloggerName;
>>> + case CLIENTAPP:
>>> + return defaultApploggerName;
>>>
>>> -
>>> - public FileLog(String fileName, boolean append) {
>>> + }
>>> + throw new RuntimeException("Unknow id " + id);
>>
>> Fix "Unknow" to "Unknown", please.
>
> Crap:(
It's the 21st century. We have spellcheckers, you know.
>>> + }
>>> +
>>> + public static enum FileLogType {
>>> +
>>
>> Formatting: Discard empty line.
>>
>>> + NETX, CLIENTAPP
>>> + }
>>> +
>>> + public static SingleStreamLogger createFileLog(FileLogType t) {
>>> + SingleStreamLogger s;
>>
>> Please use more verbose variable names instead of just t or s.
>>
>>> + try {
>>> + s = new FileLog(t);
>>> + } catch (Exception ex) {
>>> + //we do not wont to block whole logging just because
>>> initialization error
>>> + OutputController.getLogger().log(ex);
>>> + s = new SingleStreamLogger() {
>>> +
>>> + @Override
>>> + public void log(String s) {
>>> + //dummy
>>> + }
>>> +
>>> + @Override
>>> + public void close() {
>>> + //dummy
>>> + }
>>> + };
>>
>> You know that I am not much of a friend of anonymous classes. In this case,
>> DummySingleStreamLogger
>
> This was already posted as separate patch. I may ddisagree on anynoums class
> here, but It do not hurt. And - yes - Its moreover lazynes what do anonyous
> classes. So I will push the patches updated by named inner class. ok?
Hmm, after looking more closely at the net.sourceforge.jnlp.util.logging
package, now, I would rather advise to make DummySingleStreamLogger a package
level class.
>> would look just great as a nested named class, or may be even package level
>> class. ;-) It would also
>> help understanding the source code a bit more when reading.
>>
>>> + }
>>> + return s;
>>> + }
>>> +
>>> + private FileLog(FileLogType t) {
>>> + this(false, t);
>>
>> Again, variable names...
> hmhmh... IN such simple methods with quite describing type? Do you really insists?
Yep. Did you ever read the advice on Javadoc documenting public classes and
members? Basically, the same reasons apply here too.
>>> + }
>>> +
>>> + private FileLog(boolean append, FileLogType id) {
>>> + this(getNameById(id), LogConfig.getLogConfig().getIcedteaLogDir() +
>>> getPrefixById(id) +
>>> getStamp() + ".log", append);
>>> + }
>>> +
>>> + //testing constructor
>>> + FileLog(String fileName, boolean append) {
>>
>> If this is for testing, shouldn't this be private or at least protected?
>
> It is exactly why it is package private - the testis in same package. The class
> is final, so protected will not help. When it will move to private, tests will
> be not ableto create instances.
This is why we have reflection since Java 1.1. ;-) And now, you do not even need
to setup dummy constructors for testing. Isn't this great?
> So I think package private is smallest evil. But I do not insists.
> If change here, then private is an call, and then testmust be chnaged to sue
> reflection.
> That sounds correct, but as separate changeset, oook?
>
>>
>>> this(fileName, fileName, append);
>>> }
>>> -
>>> +
>>> public FileLog(String loggerName, String fileName, boolean append) {
>>> try {
>>> File futureFile = new File(fileName);
>>> if (!futureFile.exists()) {
>>> FileUtils.createRestrictedFile(futureFile, true);
>>> }
>>> - fh = new FileHandler(fileName, append);
>>> - fh.setFormatter(new Formatter() {
>>> - @Override
>>> - public String format(LogRecord record) {
>>> - return record.getMessage() + "\n";
>>> - }
>>> - });
>>> - impl = Logger.getLogger(loggerName);
>>> - impl.setLevel(Level.ALL);
>>> - impl.addHandler(fh);
>>> + bw = new BufferedWriter(new OutputStreamWriter(new
>>> FileOutputStream(new
>>> File(fileName), append), "UTF-8"));
>>
>> Why do you force UTF-8 here??? Just go with the default encoding.
>
> Ugh. I strongly disagree on default. What is benefit of using anything else
> then best?
No such thing as "the best encoding" exists. Where did you get this? :-D
> On contrary, I see may disadvantages on usinfg anything else then
> predictable encoding. At least when one sends such an log to somebody else... :-/
Well, what if the other party expects UTF-16 or UCS-2 encoded text? Sure, UTF-8
encodes all Unicode code points and so does UTF-16, and yet they are still not
interchangeable. No, keep the default here. If anybody wants or needs
IcedTea-Web to output logs in a specific encoding then there are plenty of
properties to set and other ways to accomplish this. UTF-8 is as arbitrary as
any other choice here. Some would even argue that only pure ASCII is the holy
grail of interoperability. So, go with the default encoding.
>>> log(new Header(OutputController.Level.WARNING_ALL,
>>> Thread.currentThread().getStackTrace(), Thread.currentThread(),
>>> false).toString());
>>> } catch (IOException e) {
>>> throw new RuntimeException(e);
>>> @@ -103,11 +145,25 @@
>>> */
>>> @Override
>>> public synchronized void log(String s) {
>>> - impl.log(Level.FINE, s);
>>> + try {
>>> + bw.write(s);
>>> + if (!s.endsWith("\n")){
>>
>> Fix brace formatting, please.
>>
>>> + bw.newLine();
>>> + }
>>> + bw.flush();
>>> + } catch (IOException e) {
>>> + throw new RuntimeException(e);
>>> + }
>>> }
>>> -
>>> +
>>> + @Override
>>> public void close() {
>>> - fh.close();
>>> + try {
>>> + bw.flush();
>>> + bw.close();
>>> + } catch (IOException e) {
>>> + throw new RuntimeException(e);
>>> + }
>>> }
>>>
>>> private static String getStamp() {
>>> diff -r 2682417d5671
>>> netx/net/sourceforge/jnlp/util/logging/OutputController.java
>>> --- a/netx/net/sourceforge/jnlp/util/logging/OutputController.java Thu Oct
>>> 08 11:52:14 2015 +0200
>>> +++ b/netx/net/sourceforge/jnlp/util/logging/OutputController.java Tue Oct
>>> 13 17:23:54 2015 +0200
>>> @@ -41,8 +41,10 @@
>>> import java.io.PrintStream;
>>> import java.io.PrintWriter;
>>> import java.io.StringWriter;
>>> +import java.util.Collections;
>>> import java.util.LinkedList;
>>> import java.util.List;
>>> +import java.util.regex.Pattern;
>>> import net.sourceforge.jnlp.runtime.JNLPRuntime;
>>> import net.sourceforge.jnlp.util.logging.headers.Header;
>>> import net.sourceforge.jnlp.util.logging.headers.JavaMessage;
>>> @@ -105,13 +107,19 @@
>>> private static final String NULL_OBJECT = "Trying to log null object";
>>> private PrintStreamLogger outLog;
>>> private PrintStreamLogger errLog;
>>> - private List<MessageWithHeader> messageQue = new
>>> LinkedList<MessageWithHeader>();
>>> - private MessageQueConsumer messageQueConsumer = new MessageQueConsumer();
>>> + private final List<MessageWithHeader> messageQue = new LinkedList<>();
>>> + private final MessageQueConsumer messageQueConsumer = new
>>> MessageQueConsumer();
>>> Thread consumerThread;
>>> + private final List<MessageWithHeader> applicationFileLogQueue = new
>>> LinkedList<>();
>>> + private final MessageQueConsumerForAppFileLog
>>> messageQueConsumerForAppFileLog = new
>>> MessageQueConsumerForAppFileLog();
>>> + Thread messageQueConsumerForAppFileLogThread;
>>> +
>>> /*stdin reader for headless dialogues*/
>>> private BufferedReader br;
>>>
>>> - //bounded to instance
>>> + /**bounded to instance -
>>
>> This Javadoc formatting is a bit odd.
>>
>>> + * Whole internal logging, eg MessageQueConsumer and so on is heavily
>>> synchronized. be
>>> careful changing anything on it
>>> + */
>>> private class MessageQueConsumer implements Runnable {
>>>
>>> @Override
>>> @@ -131,6 +139,35 @@
>>> }
>>> }
>>> };
>>> +
>>> + /** bounded to instance -
>>
>> Again, odd Javadoc formatting.
>>
>>> + * On contrary, MessageQueConsumerForAppFileLog is absolutely not
>>> synchronised at all.
>>> + * Synchroning it, may lead to various deadlocks with
>>> MessageQueConsumer. On contrary,
>>> + * it do not need to be synchronized as It is getting messages to queue
>>> from one thread and
>>> reads them via second.
>>> + * (no more unlike MessageQueConsumer logging stuff)
>>> + */
>>> + private class MessageQueConsumerForAppFileLog implements Runnable {
>>> +
>>> + private final Pattern rtrim = Pattern.compile("\\s+$");
>>
>> Should probably be also static.
>
>
> Oh, you are not serious, are you? The class is inner, and for reason not static.
> You can not have static variable in non static class...
Right, misread, forgot, did not realize, whatever. However, if
MessageQueConsumerForAppFileLog is quite frequently instantiated, rtrim gets as
often instantiated too, unless Pattern caches some compiled patterns, which I
would not count on. So, moving rtrim into the outer class and making it final
static may be better.
>>> +
>>> + @Override
>>> + public void run() {
>>> + while (true) {
>>> + try {
>>> + Thread.sleep(100);
>>
>> No, do not use arbitrary time slices for synchronization. This is a no go.
>
>
> Yup - this is the new sleep. Well this thread have no other synchronization then
> synchronisedList (and even it seems useless)
> As I wont to keep it as unsynchronized as posisble, then - what else you
> suggest? When you remove the sleep, it will eat incredible CPU time. And
> especially this thread should be as low priority as possible So whats the
> suggested improvement?
>
> I hope that once logging will move to writer, this seconds queue will not be
> needed. But Proof needs to wait for impl and it depnds on above four patches.
The best solution would be to use a truly asynchronous queue with callbacks.
However, you can or rather should still use Object.wait()/notify() for this purpose.
>>> + if (!(OutputController.this == null ||
>>> applicationFileLogQueue.isEmpty())) {
>>> + while (!applicationFileLogQueue.isEmpty()) {
>>> + MessageWithHeader s =
>>> applicationFileLogQueue.get(0);
>>> + applicationFileLogQueue.remove(0);
>>> +
>>> getAppFileLog().log(rtrim.matcher(proceedHeader(s)).replaceAll(""));
>>> + }
>>> + }
>>
>> Fix indentation in the try block, please.
>
> As notedabove, wil be done in separte hunks. ANyway - allmentioned rebulkes -I
> will try to asdapt to them.
>>
>>> + } catch (Throwable t) {
>>> + OutputController.getLogger().log(t);
>>> + }
>>> + }
>>> + }
>>> + };
>>>
>>> public synchronized void flush() {
>>>
>>> @@ -138,11 +175,12 @@
>>> consume();
>>> }
>>> }
>>> -
>>> +
>>> public void close() {
>>> flush();
>>> if (LogConfig.getLogConfig().isLogToFile()){
>>
>> Fix brace formatting, please.
>>
>>> getFileLog().close();
>>> + getAppFileLog().close();
>>> }
>>
>> All of close() should probably happen in cascading try-catch-finally control
>> structures. Something
>> like:
>
> I hate casding closing really more then a lot, more then personally... I really
> do. So evn during the initial patch I was inclining to autocloseable. But it
> needs to go in as separate patch.
Yeah, I personally do not like the try-catch-finally control structure at all.
It is clunky and overly verbose. Besides, one can accomplish exactly the same
stuff without it. But then, some Java language semantics would need to change...
Nevertheless, AutoCloseable isn't "automagical" either. You still have to write
the (proper) code. ;-)
>> try {
>> flush();
>> if (LogConfig.getLogConfig().isLogToFile()) {
>> getFileLog().close();
>> getAppFileLog().close();
>> }
>> } catch (Throwable t) {
>> throw t;
>> } finally {
>> try {
>> if (LogConfig.getLogConfig().isLogToFile()) {
>> if (getFileLog().close() != null) getFileLog().close();
>> if (getAppFileLog().close() != null) getAppFileLog().close();
>> }
>> } catch (Throwable t) {
>> throw t;
>> } finally {
>> if (getAppFileLog().close() != null) getAppFileLog().close();
>> }
>> }
>>
>> Note that starting with Java 7, you can use the try-with-resources control
>> structure, which makes
>> such constructs more readable. For the purpose of close(), you may also want
>> to consider for
>> OutputController to implement Closeable or AutoCloseable.
>
> Yup. +100 :) In queue!
This should suffice for now, I guess.
Jacob
More information about the distro-pkg-dev
mailing list