JDK-6774110 lock file is not deleted when child logger is used

[Stanimir Simeonoff]

On Fri, Oct 10, 2014 at 5:13 PM, Jason Mehrens <jason_mehrens at hotmail.com>
wrote:

> Hi Daniel, Stanimir,
>
>
> Closing the Handler is the main goal which takes care of the lock files.
> As far as a strong reference to the logger you don't need that.  What you
> need to do is store a reference to the Logger.handlers List in the
> LogManager$LoggerWeakRef and reap the handlers inside of dispose.
>
>
> Now the only other issue is if one handler has been added to multiple
> loggers which could close a handler early.  That is so uncommon I don't
> think it is worth the effort to correct.  The caller can just fix the code
> to use a strong reference.
>
>
> Actually we have framework that uses shared handlers quite liberally (like
writing lots of stuff in a common file) but it also keeps strong references
to any logger configured.
The framework doesn't use the properties file but a custom xml-based config.
My point is that: shared handlers may not be that uncommon and the change
may break existing code as any shared handler could be prematurely closed.

The Handler automatic closure remains problematic as they don't have a
defined lifecycle. close() should be invoked after there are no references
and it requires calling a potentially overridden method. It can be carried
by PhantomReference+WeakRefence combo, though.


>
> ----------------------------------------
> > Date: Fri, 10 Oct 2014 11:39:41 +0200
> > From: daniel.fuchs at oracle.com
> > To: stanimir at riflexo.com
> > CC: jason_mehrens at hotmail.com; core-libs-dev at openjdk.java.net
> > Subject: Re: JDK-6774110 lock file is not deleted when child logger is
> used
> >
> > Hi Stanimir, Jason,
> >
> > On 10/10/14 10:02, Stanimir Simeonoff wrote:
> >> Hi,
> >>
> >> LogManager.reset() should invoke a package private method to delete all
> >> lock files. However, that would require holding the FileHandler.locks
> >> monitor during the resetting of FileHandlers, not just the deletion
> >> process. Something like that, plus new PrivilegedAction().
> >> static void deleteAllLocks(){
> >> synchronized(locks){
> >> for (String file : locks) new File(file).delete();
> >> locks.clear();
> >> }
> >> }
> >
> > There's more than the deletion of the lock file unfortunately. I believe
> > the handlers should be properly closed. A handler with an XMLFormatter
> > for instance needs to write the tail of the file.
> >
> >
> >> Alternatively the deletion could just be part of the Cleaner
> >> shutdownhook with another sun.misc.Cleaner per FileHandler that deletes
> >> the file. (Handlers can be shared amongst loggers, so they cannot be
> >> closed explicitly). There is a certain risk as file.delete() can be a
> >> very slow operation, though (ext3 [concurrently] deleting large files
> >> for example).
> >
> > That's a solution I envisaged and rejected because of the constraints
> > we have when running in the ReferenceHandler thread. I don't think it
> > would be appropriate to close a Handler in that thread.
> >
> > I'm leaning towards suggesting that the LogManager should hold a strong
> > reference on the loggers for which a Handler is explicitly
> > configured in the configuration file. It would ensure that
> > these loggers are still around when reset() is called.
> >
> > best regards,
> >
> > -- daniel
> >
> >>
> >> Stanimir
> >>
> >>
> >>
> >> On Thu, Oct 9, 2014 at 4:59 PM, Daniel Fuchs <daniel.fuchs at oracle.com
> >> <mailto:daniel.fuchs at oracle.com>> wrote:
> >>
> >> Thanks Jason.
> >>
> >> I wonder if that may be another issue. Interesting. I'll see if I
> >> can work out a test case
> >> for that tomorrow. With the test case provided in the bug - tested
> >> on 7, 8, and 9,
> >> the only file that remained at the end was 'log' (which is as it
> >> should be - and I ran
> >> the test case several times with each JDK) - which lets me think
> >> that maybe the
> >> issue was different.
> >>
> >> Now what you describe looks indeed like a bug that should still be
> >> present
> >> in the code base. I didn't think about that scenario, thanks for
> >> pointing it out!
> >> If i can write a reproducer (which should not be too difficult), it
> >> will be a good
> >> incentive to attempt a fix :-)
> >>
> >> Thanks again,
> >>
> >> -- daniel
> >>
> >>
> >> On 10/9/14 9:56 PM, Jason Mehrens wrote:
> >>
> >> Daniel,
> >>
> >>
> >> The evaluation on this bug is not quite correct. What is going
> >> on here is the child logger is garbage collected which makes the
> >> FileHandler unreachable from the LogManager$Cleaner which would
> >> have closed the attached FileHandler. In the example, there is
> >> no hard reference that escapes the 'execute' method. Prior to
> >> fixing JDK-6274920: JDK logger holds strong reference to
> >> java.util.logging.Logger instances, the LogManager$Cleaner would
> >> have deleted the lock file on shutdown. Now that the loggers
> >> are GC'able, one possible fix would be change the
> >> FileHandler.locks static field to Map<String,FileHandler> where
> >> the key is the file name and the value is the FileHandler that
> >> is open. Then in the LogManager$Cleaner could close any entries
> >> in that map after LogManager.reset() is executed.
> >>
> >>
> >> Jason
> >>
> >>
> >>
> >
>