Go for it! Ralph
> On May 4, 2014, at 7:04 PM, Bruce Brouwer <[email protected]> wrote: > > 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 <[email protected]> >> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> 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 <[email protected]> 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" <[email protected]> wrote: >>>>>>> If you've got a good idea on how to do it, sure. >>>>>>> >>>>>>> >>>>>>>> On 4 May 2014 14:04, Bruce Brouwer <[email protected]> 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" <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >>>>>>>>>>> No, it can be simpler than that. >>>>>>>>>>> >>>>>>>>>>> Sent from my iPad >>>>>>>>>>> >>>>>>>>>>>> On May 4, 2014, at 10:55 AM, Matt Sicker <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 >>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>> Hooray, we've finally figured out the bug. :) >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 3 May 2014 19:49, Remko Popma <[email protected]> >>>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>>>> I just fixed it in r1592291 haha >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On 3 May 2014 17:54, Ralph Goers >>>>>>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Does closing them do anything? >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> On 3 May 2014 17:10, Ralph Goers >>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>>>>>>> Perhaps we need a StatusFileListerner when writing to a >>>>>>>>>>>>>>>>>>>>>> file? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Ralph >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> On May 3, 2014, at 3:03 PM, Ralph Goers >>>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> System.out or System.err should never be closed. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Ralph >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> On May 3, 2014, at 10:59 AM, Matt Sicker >>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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 <[email protected]> >>>>>>>>>>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yes, windows 7. >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Sun, May 4, 2014 at 12:54 AM, Ralph Goers >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> <[email protected]> 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: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For additional commands, e-mail: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Bruce Brouwer >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Matt Sicker <[email protected]> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Matt Sicker <[email protected]> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Matt Sicker <[email protected]> >>>>> >>>>> >>>>> >>>>> -- >>>>> Matt Sicker <[email protected]> >>> >>> >>> >>> -- >>> >>> Bruce Brouwer > > > > -- > > Bruce Brouwer
