Community
Participate
Working Groups
Created attachment 96621 [details] ExtendedLogReaderServiceFactory synchronization patch ConcurrentModificationExceptions are thrown when using the extended log service (incubator org.eclipse.equinox.log) when one thread adds a log listener while another thread is making a log entry. Addition of synchronization of the ExtendedLogReaderServiceFactory's listerners map (see patch) will solve this problem.
Thanks Paul. I'll take a look and commit this later on today or tommorrow. Sort of related... I have a synchronized task queue sitting in my workspace for sending log entries to listeners on a different thread as per the spec. To avoid any breakage for thread-local MDC stuff (I seem to recall you using this) I'll set it up disabled by default via flag. I'd appreciate it if you could take a look.
Sure. Currently I'm only using MDC in a log4j LogListener to set the bundle name. When I get around to updating my slf4j binding to 1.5 (likely soon), I'll be able to test MDC end-to-end. Another little thing that needs changing in this bundle is ExtendedLogServiceFactory.getLogger(). I'm caching loggers in my slf4j binding, but I think it should be done here instead.
Contributed patch applied. Thanks Paul. I added the task queue but didn't do anything fancy with it yet. I think we're eventually going to have to do something better for synchronization in this class because of the volume of potentially concurrent requests. A rwlock seems pretty obvious but I also had wondered about caching filter decisions if we stipulated that a decision was static. e.g. if you want to change the decision of your filter you would have to re-register your listener. You're right, caching loggers is important. It maybe can be done in the ExtendedLogServiceImpl which should make cleanup easy.
I originally used a ConcurentHashMap, but then took it out before I sent it to you, because I'm not sure what the execution environment has to be. Does the extended log service need to be M1.1? The java.util.concurrent stuff would give you a synchronous queue also, but I guess that's wishful thinking. Otherwise, I can add a r/w lock. I don't think MDC is going to be a problem. When an entry is logged, the data has to be sent through as non-TLS data, and turned back into TLS once it hits the log listener, which does the "real" MDC logging. > filter decisions if we stipulated that a decision was static. e.g. if you want > to change the decision of your filter you would have to re-register your > listener. I think there are valid use cases for dynamic loggers where you really can't cache the decisions. How about setting a service property for a cache strategy, with the default being 'cache decisions'? > You're right, caching loggers is important. It maybe can be done in the > ExtendedLogServiceImpl which should make cleanup easy. It delegates the work to ExtendedLogServiceFactory, so as it stands now, it would have to go in the factory. However, both the ExtendedLogServiceFactory and ExtendedLogReaderSerivceFactory are acting as the logservice/log reader service. Is that a hangover from the original org.eclipse.equinox.log bundle? Since you're adding in the task thread part now, would it make sense to refactor those classes?
