So, who gets to implement it. Ralph, is your time better spent on other
log4j issues, or should I look for new opportunities to help here? Any
suggestions?
On Sun, May 4, 2014 at 9:25 PM, Ralph Goers <ralph.go...@dslextreme.com>wrote:
> It sounds like we are on a similar approach.
>
> Even with registering the Configuration with the StatusFileListener it is
> possible that multiple configurations would want to write to the same file,
> and so should share the same Listener. In that case closing the file would
> only happen when the last Configuration is unregistered.
>
> Ralph
>
> On May 4, 2014, at 6:06 PM, Bruce Brouwer <bruce.brou...@gmail.com> wrote:
>
> I was thinking of something along these lines (since Matt was asking me
> for my ideas):
>
> First off, make StatusConsoleListener a base class for
> StatusSystemOutListener and StatusSystemErrListener (maybe inner classes).
> These would no longer need to hold a reference to System.out or System.err,
> so they will implictly follow any changes to System.out or System.err.
>
> Then make a new StatusFileListener for handling files and this class would
> know to close output streams. This class could hold a static map of
> listeners. We could either do reference counting here or my preference -
> register the Configuration with StatusFileListener to get the listener. e.g.
>
> static StatusFileListener register(Configuration config, String fileName)
> { ... }
> static void StatusFileListener release(Configuration config) { ... }
>
> In the XMLConfiguration (and JSONConfiguration) stop and reconfigure
> methods, it would deal with releasing the config. This is hidden behind
> StatusConfiguration, so it might involve XMLConfiguration and
> JSONConfiguration maintaining a reference to StatusConfiguration. I haven't
> totally decided yet what would be best.
>
> (I was in the middle of typing this when I got Ralph's response)
>
>
> On Sun, May 4, 2014 at 9:03 PM, Ralph Goers <ralph.go...@dslextreme.com>wrote:
>
>> I plan on removing the “extends Closeable” from the StatusListener
>> interface and then creating a StatusCloseableListener that extends
>> StatusConsoleListener. The close method will move there. Then in
>> StatusConfiguration we will create the appropriate type based on what the
>> destination is set to. I may choose to create a static map in
>> StatusCloseableListener and keep a counter for each destination name or I
>> may just use the property that says whether it is a web container and not
>> close the files if it is. I am not sure yet which would be the best option.
>>
>> Ralph
>>
>> On May 4, 2014, at 4:30 PM, Matt Sicker <boa...@gmail.com> wrote:
>>
>> Could you summarize how you'd implement this? Idea sharing could help
>> reduce the overall effort required to elegantly fix this.
>>
>>
>> On 4 May 2014 16:00, Bruce Brouwer <bruce.brou...@gmail.com> wrote:
>>
>>> I was hoping to hear from Ralph as it sounds like he already started
>>> something.
>>> On May 4, 2014 3:22 PM, "Matt Sicker" <boa...@gmail.com> wrote:
>>>
>>>> If you've got a good idea on how to do it, sure.
>>>>
>>>>
>>>> On 4 May 2014 14:04, Bruce Brouwer <bruce.brou...@gmail.com> wrote:
>>>>
>>>>> I haven't spent much time on this since my initial attempt on 609.
>>>>> Shall I leave it to Ralph to come up with the final solution, or would you
>>>>> like me to try?
>>>>> On May 4, 2014 2:35 PM, "Matt Sicker" <boa...@gmail.com> wrote:
>>>>>
>>>>>> Looks like there's nothing to synchronise on actually. Guess you can
>>>>>> just cache them before the check in general.
>>>>>>
>>>>>>
>>>>>> On 4 May 2014 13:25, Matt Sicker <boa...@gmail.com> wrote:
>>>>>>
>>>>>>> Oh phew. Well, I'll leave that to you if you wanted to continue what
>>>>>>> you were working on. All I added was a check on close() to compare
>>>>>>> against
>>>>>>> the current System.out and System.err. I'll take a look into OpenJDK to
>>>>>>> see
>>>>>>> how to properly lock those (if possible) to prevent fun race conditions.
>>>>>>>
>>>>>>>
>>>>>>> On 4 May 2014 13:11, Ralph Goers <rgo...@apache.org> wrote:
>>>>>>>
>>>>>>>> No, it can be simpler than that.
>>>>>>>>
>>>>>>>> Sent from my iPad
>>>>>>>>
>>>>>>>> On May 4, 2014, at 10:55 AM, Matt Sicker <boa...@gmail.com> wrote:
>>>>>>>>
>>>>>>>> This is starting to sound like we need a full-blown
>>>>>>>> factory/context/logger implementation of StatusLogger.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4 May 2014 12:46, Ralph Goers <rgo...@apache.org> wrote:
>>>>>>>>
>>>>>>>>> Also, that doesn't solve the case Remko mentioned of multiple web
>>>>>>>>> apps writing to a single file.
>>>>>>>>>
>>>>>>>>> Ralph
>>>>>>>>>
>>>>>>>>> On May 4, 2014, at 9:53 AM, Matt Sicker <boa...@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> So how about adding a check at construction checking against
>>>>>>>>> System.out and System.err? Really, once you start messing with those
>>>>>>>>> streams, you can't be sure they'll ever be closed until the JVM stops
>>>>>>>>> or
>>>>>>>>> you manually close it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 4 May 2014 09:36, Ralph Goers <rgo...@apache.org> wrote:
>>>>>>>>>
>>>>>>>>>> I see two choices here - maintain a use count or just let the OS
>>>>>>>>>> close the files.
>>>>>>>>>>
>>>>>>>>>> The second would be pretty easy to do once we move the web stuff
>>>>>>>>>> to its own module as it can add a property that the console Appender
>>>>>>>>>> would
>>>>>>>>>> look for.
>>>>>>>>>>
>>>>>>>>>> The first option is probably better if it could be made to work
>>>>>>>>>> properly.
>>>>>>>>>>
>>>>>>>>>> Ralph
>>>>>>>>>>
>>>>>>>>>> On May 4, 2014, at 6:38 AM, Bruce Brouwer <
>>>>>>>>>> bruce.brou...@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> This is what I was starting to investigate with LOG4J2-609.
>>>>>>>>>>
>>>>>>>>>> I don't think this is quite there yet. For one, in
>>>>>>>>>> StatusConsoleListener.close(), System.out and System.err can change
>>>>>>>>>> over
>>>>>>>>>> time, so doing the != check might still close something that at one
>>>>>>>>>> time
>>>>>>>>>> was System.out but no longer is.
>>>>>>>>>>
>>>>>>>>>> Also, a StatusConsoleListener is shared among different
>>>>>>>>>> JSONConfiguration and XMLConfiguration instances (think about
>>>>>>>>>> multiple WARs
>>>>>>>>>> in a Tomcat instance where log4j is in Tomcat's shared lib
>>>>>>>>>> directory). If
>>>>>>>>>> we undeployed one of those WARs, it would shutdown the
>>>>>>>>>> StatusConsoleListener that was shared with the other WAR deployments.
>>>>>>>>>>
>>>>>>>>>> Also think about where some of these WARs wanted to use
>>>>>>>>>> System.out and others want to use a log file for status logging.
>>>>>>>>>> Because of
>>>>>>>>>> the way these shared loggers are found, only the first
>>>>>>>>>> StatusConsoleListener registered would actually take effect. So
>>>>>>>>>> sometimes
>>>>>>>>>> when you start Tomcat, status logs go to System.out, other times
>>>>>>>>>> they go to
>>>>>>>>>> a log file. I'd hate having to debug that one if I didn't know about
>>>>>>>>>> this
>>>>>>>>>> issue.
>>>>>>>>>>
>>>>>>>>>> I have an idea of how to address this, but it unfortunately isn't
>>>>>>>>>> as simple as closing the StatusConsoleListener.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Sat, May 3, 2014 at 10:04 PM, Matt Sicker <boa...@gmail.com>wrote:
>>>>>>>>>>
>>>>>>>>>>> Hooray, we've finally figured out the bug. :)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 3 May 2014 19:49, Remko Popma <remko.po...@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> I just updated from SVN and all tests now pass.
>>>>>>>>>>>> The build works now. Thanks!
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Sun, May 4, 2014 at 7:55 AM, Matt Sicker
>>>>>>>>>>>> <boa...@gmail.com>wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> I just fixed it in r1592291 haha
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 3 May 2014 17:54, Ralph Goers
>>>>>>>>>>>>> <ralph.go...@dslextreme.com>wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes. It cause them to close. Anything written to System.out
>>>>>>>>>>>>>> or System.err will fail.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On May 3, 2014, at 3:51 PM, Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Does closing them do anything?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 3 May 2014 17:10, Ralph Goers
>>>>>>>>>>>>>> <ralph.go...@dslextreme.com>wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Perhaps we need a StatusFileListerner when writing to a file?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ralph
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On May 3, 2014, at 3:03 PM, Ralph Goers <
>>>>>>>>>>>>>>> ralph.go...@dslextreme.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> System.out or System.err should never be closed.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ralph
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On May 3, 2014, at 10:59 AM, Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've implemented Closeable on StatusListener in r1592258.
>>>>>>>>>>>>>>> Please try out the unit tests again and let me know if this
>>>>>>>>>>>>>>> solves the
>>>>>>>>>>>>>>> issue on Windows.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 3 May 2014 12:30, Matt Sicker <boa...@gmail.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I think this is actually a bug. StatusListener should
>>>>>>>>>>>>>>>> implement Closeable, and when the listeners are cleared, it
>>>>>>>>>>>>>>>> should loop
>>>>>>>>>>>>>>>> through and close them before clearing the list of listeners.
>>>>>>>>>>>>>>>> Otherwise,
>>>>>>>>>>>>>>>> files can stay opened and Windows still hasn't figured out how
>>>>>>>>>>>>>>>> to handle
>>>>>>>>>>>>>>>> that.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 3 May 2014 11:22, Remko Popma <remko.po...@gmail.com>wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks, commenting out that test to verify my changes was
>>>>>>>>>>>>>>>>> exactly what I was doing now... :-)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Sun, May 4, 2014 at 1:20 AM, Ralph Goers <
>>>>>>>>>>>>>>>>> ralph.go...@dslextreme.com> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Oh, and if you are trying to do some work just comment
>>>>>>>>>>>>>>>>>> out the @Test of the failing test - but don’t commit that.
>>>>>>>>>>>>>>>>>> Ralph
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On May 3, 2014, at 9:19 AM, Ralph Goers <
>>>>>>>>>>>>>>>>>> ralph.go...@dslextreme.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> That happens because the file is still being referenced
>>>>>>>>>>>>>>>>>> by something when it is trying to delete it. It should be
>>>>>>>>>>>>>>>>>> because the file
>>>>>>>>>>>>>>>>>> is open but I recall reading that Windows sometimes holds on
>>>>>>>>>>>>>>>>>> to file
>>>>>>>>>>>>>>>>>> references longer than it should. This was probably caused
>>>>>>>>>>>>>>>>>> by the changes
>>>>>>>>>>>>>>>>>> Matt made to the unit test framework a month or so ago. I
>>>>>>>>>>>>>>>>>> will bring up my
>>>>>>>>>>>>>>>>>> Windows VM and take a look at it this afternoon.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Ralph
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On May 3, 2014, at 8:58 AM, Remko Popma <
>>>>>>>>>>>>>>>>>> remko.po...@gmail.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yes, windows 7.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Sun, May 4, 2014 at 12:54 AM, Ralph Goers <
>>>>>>>>>>>>>>>>>> ralph.go...@dslextreme.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> FileOutputTest was failing for me last week and I
>>>>>>>>>>>>>>>>>>> thought I fixed it. But it was failing because the file was
>>>>>>>>>>>>>>>>>>> empty, not
>>>>>>>>>>>>>>>>>>> because it couldn’t be deleted. I guess you must be running
>>>>>>>>>>>>>>>>>>> on Windows?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Ralph
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On May 3, 2014, at 8:44 AM, Remko Popma <
>>>>>>>>>>>>>>>>>>> remko.po...@gmail.com> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> > When I run mvn clean install, I get this problem:
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > Failed tests:
>>>>>>>>>>>>>>>>>>> > FileOutputTest.testConfig Could not delete
>>>>>>>>>>>>>>>>>>> target\status.log, last modifed 14/05/04 0:27
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > FileOutputTest has a "CleanFiles" rule that seems to
>>>>>>>>>>>>>>>>>>> fail:
>>>>>>>>>>>>>>>>>>> > public RuleChain rules = RuleChain.outerRule(new
>>>>>>>>>>>>>>>>>>> CleanFiles(STATUS_LOG)).around(new
>>>>>>>>>>>>>>>>>>> InitialLoggerContext(CONFIG));
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > How do I fix this?
>>>>>>>>>>>>>>>>>>> >
>>>>>>>>>>>>>>>>>>> > Remko
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> ---------------------------------------------------------------------
>>>>>>>>>>>>>>>>>>> To unsubscribe, e-mail:
>>>>>>>>>>>>>>>>>>> log4j-dev-unsubscr...@logging.apache.org
>>>>>>>>>>>>>>>>>>> For additional commands, e-mail:
>>>>>>>>>>>>>>>>>>> log4j-dev-h...@logging.apache.org
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>>
>>>>>>>>>> Bruce Brouwer
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <boa...@gmail.com>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <boa...@gmail.com>
>>>>
>>>
>>
>>
>> --
>> Matt Sicker <boa...@gmail.com>
>>
>>
>>
>
>
> --
>
> Bruce Brouwer
>
>
>
--
Bruce Brouwer
