james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Burrell Donkin" <robertburrelldon...@gmail.com>
Subject Re: [JSieve] Sporadic Mysterious Exceptions
Date Thu, 09 Aug 2007 16:30:51 GMT
On 8/9/07, Stefano Bagnara <apache@bago.org> wrote:
> Bernd Fondermann ha scritto:
> > What types of exceptions do you get? NPE?

mostly ones related to the state machine. it's the same script and it
filters the mail fine in a single threaded test environment but
occasionally fails when run from the spool.

> > 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!!

+1

probably better to replace lazy construction with a final variable

> > 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 ?

good spot 8-)

+1

probably better to use a stack and push the implementation into ConditionManager

> Shouldn't
> resetInstance simply remove the conditionManager from the fieldInstance
> ThreadLocal  using a fieldInstance.set(null) ?

i've been running with this change in for a couple of days and (as
yet) there haven't been any exceptions thrown

- robert

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message