Thanks for guidance, I will try it later. Reverted in revision: 728536
Jacques
From: "Adam Heath" <[email protected]>
Jacques Le Roux wrote:
Hi Adam,
I don't mind to revert this patch, but I think it's nice to have this
feature.
I suppose that what you call "multiple calls in series" are expressions
like (and yes it's not synchronized, my bad !)
+ for (Appender appender : appenders) {
+ if (appender != null &&
appender.getName().equals(appenderName)) {
+ return appender;
+ }
No, that's not what I mean.
Could we not replace Vector (bad I agree) by synchronizedMap and use
synchronization on the synchronized Map, like in
http://java.sun.com/j2se/1.4.2/docs/api/java/util/Collections.html#synchronizedList(java.util.List)
Foo hasFoo() {
synchronized (fooContainer) {
}
}
void setFoo() {
synchronized (fooContainer) {
}
}
Foo getFoo() {
if (!hasFoo()) setFoo();
return getFoo();
}
This is the pattern that is in this patch, and it's a race condition.
In addition, synchronizing on the internal Collection(the value in the
map) isn't always worth it, if you are already inside a synchronized
block on some other variable.
For the memorty leaks, I think removeAppenderFromThreadGroupMap would be
OK, isn'it ?
Integer.toString(), not "" + int.
is bad I agree
Also here result is not genericized as it was "before" (but I guess the
patch was anterior too the genericization, and I did not spot that also,
maybe there are more I will be carefull on this aspect too)
- Map<String, Object> result =
dispatcher.runSync(getServiceName(), getContext());
+
+ if (this.logLocation != null) {
+ Debug
+ .registerCurrentThreadGroupLogger(this.logLocation,
+ appenderName);
+ }
+ Map result = dispatcher.runSync(getServiceName(),
getContext());
So, by usint also FlexibleLocation instead of File, don't you think we
could amend this "patch" instead of simply reverting it ?
No, because there are issues with memory loss too, due to the
reimplementation of what ThreadLocal actually accomplishes.
What would be better, is to implement a singleton per-thread
initialization and cleanup system.
==
void ThreadWorker.run() {
try {
doWork();
} finally {
ThreadInit.runCleanup();
}
}
static <T> T ThreadInit.getInitObject(ThreadInit<T> init) {
Map<Class<? extends ThreadInit>, InitData> initMap = tl.get();
if (initMap == null) {
initMap = FastMap.newInstance();
tl.set(initMap);
}
InitData<T> initData = initMap.get(init.getClass());
if (initData != null) return initData.getValue();
T initValue = init.start();
initMap.put(init.getClass(), new InitData<T>(init, initValue));
return initValue;
}
static void ThreadInit.runCleanup() {
Map<Class<? extends ThreadInit>, InitData<?>> initMap = tl.get();
if (initMap == null) return;
for (InitData<?> initData: initMap.values()) {
runCleanup(initData);
}
}
static <T> void ThreadInit.runCleanup(InitData<T> initData) {
initData.getInit().runCleanup(initData.getValue());
}
==
This allows any random code anywhere to have init code that runs *once*
per thread invocation; it just requires that very early in the
invocation chain, is a try/finally block, that runs the cleanup code.
It's also possible to extend this slightly, so there is a concept of
push/pop ThreadLocal state.
You'll note there is *no* synchronization here. This is because the
top-level variable that holds the object graph, is a ThreadLocal.
Synchronization is only used for cross-thread control. ThreadLocal is
local, and can't corrupt across threads.
