[rfc][icedtea-web] pre-review - accidental logging tweeking
Jiri Vanek
jvanek at redhat.com
Fri Oct 16 17:56:44 UTC 2015
On 10/16/2015 04:52 PM, Jacob Wisor wrote:
> 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!
...
>> 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?
I didn't?
>
>> 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.
>>>
...
>>>> 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.
What should I do? There were patches o review for month around june. So my waiting period if somebody will grap one was getting lower and lower. Until current 1 day for very small pathces.
Still - i pushed *only* the very small part the fix to initialization error. To this part you complained on two hunks, And I fexed them both.
In meanwhile I posted other three parts (in two patches) as separate emails.
And TBH I'm the only surviving active ITW develope. I'm terribly sorry for that, but stil there are some goals I wont to achieve. Waiting for month for review will cause the 1.7 to be release in decades. I promiss 1.7 will be my last release.
Speaking about releases, security pathcing and similar - how can you blame me for working on them? I'm actually only person around keeping itw somehow alive :( And I'm not proud nor happy form it.
And if taking you personally - I can not relay on people connecting absolutley randomly. I understand why you do so. And I apologise for inpatients. But I really do nto wont to have longer cycles then year:(
>
>>> 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.
And what other way? Nobody else is wokrking on ITW.
I highly approciate your input, but real code? You do not post ones:( I will always follow different advice how to solve problem. But very often you only say that this should not be addressed like this. But dont offer an next solution.
>
>> 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
I really do not wont reworking of date schema here.
But
a) it is not part of this patch.
b) I don't have cycles for it. Feel free to post patch,m and I will be happy to review it.
c) the timezone is highly discussable. see lower.
> 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.
Really not. I was following the convention ITW had as closely as possible. SO instead of my "good gut" please di history of this format in repo.
>
>>> 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!
Ok. I understand your point, but I don't think itw is this kind of software.
If you won tot integrate better system logging - please go on. It can be even windows-only. I will be happy for it.
Secondly - you you stated some facts. About admins and timezones. Be sure I understand this. But is ITW this kind of application?
I doubt it. I would like to move the discussion about the format to different thread with pros/coins and possibly also patch. And with wider audience. because accoridng to it you think + and me myself -.
>
>> 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.
same as above.
>
>>> 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.
Ugh. I really disagree very strongly. But without patch those all are jsut plain words.
>
>>> 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
but it IS pluginSharedFormatter MORE then logFileNameFormatter
Best would be logFileNameFormatteWithFormatSharedWithPlugin but thats terrible...
>
> 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.
fixed in later posted separated patches,
>
>>>> + 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.
There is absolutely nothing I can say for my defewnce and thank you for catching it.
Anyway I abandoned this approach in the new set of patches.
>
>>>> + }
>>>> +
>>>> + 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.
I kept it anonyomous. UNless it will be reused, I would keep it like it. But I f yo insists, I will fix.
>
>>> 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.
true
>
>>>> + }
>>>> +
>>>> + 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?
ugh. I'm not sure. I think java world is split 50/50 bit-dirty-api/refelction.
>
>> 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.
This patch is still on review. In whole my career I rember several cases where we were moving code to what i worte, but never back to default.
I will try to find some older ITW review to split the stone.
>
>>>> 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.
Sure. This hunk is currentlky also abandoned.
>
>>>> +
>>>> + @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.
Or no queue at all.
>
>>>> + 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. ;-)
cool. Here we agree :)
>
>>> 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