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>
