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
