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
