I agree to a previous post that the whole construct is very hard to
get and a little bit overengineered. +1 to try and remove ThreadLocal.
+1 to at least use a subclass. +1 to reduce complexity by inlining
methods or other appropriate refactorings.
Bernd
On 8/9/07, Stefano Bagnara <[EMAIL PROTECTED]> wrote:
> Bernd Fondermann ha scritto:
> > What types of exceptions do you get? NPE?
> >
> > if it wasn't for a ThreadLocal, I'd say that this method's init is not
> > atomic, if fieldInstance is NULL:
> >
> > static protected void setInstance(CommandStateManager conditionManager)
> > {
> > if (null == fieldInstance)
> > fieldInstance = new ThreadLocal();
> > fieldInstance.set(conditionManager);
> > }
> >
> > It leaves ThreadLocal uninitialized but accessible for a little
> > moment. if the method got interrupted in between and getInstance()
> > gets executed, this might result in a NPE.
>
> Good catch!!
>
> > But maybe this case will never occur because of the special nature of
> > ThreadLocals?
>
> I don't think so, but your pointer brought me to look for callers of
> that code, and here it is:
>
> /**
> * Method execute executes a Block within the context of a new
> ConditionManager.
> * @param mail
> * @param block
> * @return Object
> * @throws SieveException
> */
> protected Object execute(MailAdapter mail, Block block) throws
> SieveException {
> // Switch to a new ConditionManager
> ConditionManager oldManager = ConditionManager.getInstance();
> ConditionManager.resetInstance();
>
> // Execute the Block
> Object result = block.execute(mail);
>
> // Restore the old ConditionManager
> ConditionManager.setInstance(oldManager);
>
> return result;
> }
>
> I don't get how it works. ConditionManager.resetInstance will set the
> static thread local fieldInstance to null: doesn't this invalidate also
> instances of every other thread using the ConditionManager ? Shouldn't
> resetInstance simply remove the conditionManager from the fieldInstance
> ThreadLocal using a fieldInstance.set(null) ?
>
> Stefano
>
> > On 8/6/07, Robert Burrell Donkin <[EMAIL PROTECTED]> wrote:
> >> 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?
> >>
> >> can anyone think of any better approaches?
> >>
> >> - robert
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [EMAIL PROTECTED]
> >> For additional commands, e-mail: [EMAIL PROTECTED]
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [EMAIL PROTECTED]
> > For additional commands, e-mail: [EMAIL PROTECTED]
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
