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.
