logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject [Bug 41214] Deadlock with RollingFileAppender
Date Mon, 22 Oct 2012 17:29:25 GMT
https://issues.apache.org/bugzilla/show_bug.cgi?id=41214

--- Comment #47 from Jason Marshall <jdmarshall@gmail.com> ---
After discussion with some colleagues, I would like to propose the following
fix:

You cannot protect AppenderAttachableImpl from here.  You can only protect the
lazy instantiation.   All of the public methods on AppenderAttachableImpl
should be marked synchronized if you're worried about add and remove calls
causing problems.

        Category c = this
        while (c != null) {
            Category parent;
            boolean additive;
            AppenderAttachableImpl aai;

            // Protected against simultaneous call to addAppender,
removeAppender,...
            synchronized(c) {
                aai = c.aai;
                additive = c.additive;
                c = c.parent();
            }

            if(aai != null) {
                writes += aai.appendLoopOnAppenders(event);
            }

            if(!additive) {
                break;
            } 
        }

        if(writes == 0) {
            repository.emitNoAppenderWarning(this);
        }
    }

With this code, no lock is held on the heirarchy while toString can be called,
and no two locks are held at once, breaking the deadlock race condition. 
In-flight log messages can still show up in the old log while structural
changes are being made, but I believe that was always a danger anyway.

Thoughts?

(In reply to comment #46)
> I can see the deadlock from here.
> 
> With nested Categories A->B->C
> 
> While you're in B.callAppenders B is locked.  If toObject() attempts to log,
> then C.callAppenders is invoked again.  If someone else has already called
> C.callAppenders, you have a deadlock.  One has C waiting for B, another has
> B waiting for C.  
> 
> The classical solution to this problem is to sort your locks and grab them
> in the same predictable order.  The parent-child relationship -almost-
> solves the problem here, except for a couple problems.  One, being able to
> rearrange your loggers while logging is happening, and the other is that
> pesky toObject() method.
> 
> What you probably should do here is call parent.callAppenders while still
> holding your own lock. With deeply nested Categories this will widen your
> bottleneck but eliminate the deadlock.  I won't be able to write to c.log
> while someone is still writing to b.log or a.log, and unfortunately it also
> breaks your emitNoAppenderWarning fallback, because callAppenders returns
> void.  
> 
> It's unclear to my why the parentage of a Category can be updated after it's
> created.  It seems like it's an artifact of the evolution of the library. 
> I'm not familiar with this aspect of Log4j but it seems that this is a case
> of premature publication.  Add on a sprinkle of too much delegation and a
> dash of concurrency and you've got quite a soup here.
> 
> I thought I had a patch proposal (that didn't break the API) but it turned
> out on further inspection to have the exact same failure mode.  So instead
> of my help all I can offer is my sympathy.  I now understand why this has
> been open for so long.  
> 
> I do think that if you pull some of the deprecated things out of this part
> of the API that it may be easier to fix.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org


Mime
View raw message