Robert Burrell Donkin ha scritto:
> i've been observing sporadic exceptions in my fetchmail->sieve->IMAP.
> the exceptions are thrown even though the mail filtering works fine
> when the script is run offline. the majority of these exceptions have
> been fixed by adding read/write locking to the torque mailbox. so,
> i've been able to focus on the small number left. these don't appear
> to be IMAP related.
> 
> the usual symptom (for these remaining issues) is a failed check in
> http://svn.apache.org/repos/asf/james/jsieve/trunk/src/main/java/org/apache/jsieve/commands/CommandStateManager.java.
>  i've been wondering recently whether the concurrency isn't quite
> right in this class. these issues are hard to debug and tend to
> require a lot of staring at the code. so hopefully, people will help
> me out by lending an eye ball or two and then stating their opinions.
> 
> 1 ThreadLocal is created lazily and is used without subclassing.
> http://java.sun.com/j2se/1.5.0/docs/api/java/lang/ThreadLocal.html
> recommends subclassing.
> 
> 2 the instance is set to nulled on a reset. in the case of concurrent
> threads, a reset in one thread may lead to loss of state in another. a
> reset on the instance would prevent this.
> 
> i think that it may better to eliminate the static per-thread
> singleton manipulation methods and replace them with a ThreadLocal
> subclass declared final. the reset would then be moved into the
> instance.
> 
> opinions?

I see the set/update/compute/reset/get/getBasic pattern applied there.
This is a Steve signature ;-) . I admit I don't know the motivation
behind that pattern and I don't know what are the advantages and
problems caused by that pattern. I tend to remove most of them when I
find it on my way because of code readability issues IMHO sacrificed
with no specific advantage (but maybe it's my ignorance).

I rarely used ThreadLocals in past but when I used it I referred to the
way documented by Concurrent programming in Java.
I don't have "synchronized" in the getInstance (that simply call the
threadlocal.get) and the threadlocal is not subclassed (but a simple
private static, like the one in CommandStateManager).

That said I would probably try to simplify the pattern used in the class
 by splitting the threadlocal container by the CommandStateManager. Here
we have a weird behavior where each instance contains another unused
threadlocal creating a "new CommandStateManager();".

Maybe it should be refactored so that the object containining the
statuses (and setters/getters) is an inner static class and does not
contain another ThreadLocal.

Another issue I see with ThreadLocal usage is that this may conflict
with SEDA based "users" of the library. I don't know the jSieve
architecture but if it keep 1 status for each thread then we have to be
sure you initialize and complete a full jSieve call within a single
thread. Otherwise we would need a Memento pattern instead of the
ThreadLocal so that the client could pass the current status at each call.

Again I never used and I don't know jSieve codebase, so this is only a
consideration based on reading few java source code.

Stefano


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to