You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by Robert Burrell Donkin <ro...@gmail.com> on 2007/08/06 21:34:42 UTC
[JSieve] Sporadic Mysterious Exceptions
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: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
> > On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> >> Stefano Bagnara ha scritto:
> >>> /**
> >>> * 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) ?
> >> Forgot to say that if this is the case it should be pretty easy to
> >> create a test case to prove it.
> >
> > how would you approach the management of the multiple threads?
>
> I'm writing the code in thunderbird, so forget syntax and other
> problems. Hope you get the idea:
>
> void testIssue() {
> ConditionManager instance1 = ConditionManager.getInstance();
>
> Thread otherThread = new Thread() {
> public void run() {
> ConditionManager.resetInstance();
> }
> }.start();
>
> otherThread.join();
>
> assertEquals(instance1, ConditionManager.getInstance());
> }
>
> otherwise, instead of checking the ConditionManager methods directly we
> could check the call to the AbstractConditionalCommand.execute.
>
> get the manager, get the condition manager instance, run the execute in
> the main thread, run the execute of another thread using the same
> pattern above, check the condition manager instance (it has to be the
> same again).
that'd probably do it
> Of course this simply prove this issue and does not check for every
> synchronization error, but I find it useful to prove bugs with tests and
> see them greened by the patch.
true
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
> > On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
<snip>
> >> Forgot to say that if this is the case it should be pretty easy to
> >> create a test case to prove it.
> >
> > how would you approach the management of the multiple threads?
>
> I'm writing the code in thunderbird, so forget syntax and other
> problems. Hope you get the idea:
>
> void testIssue() {
> ConditionManager instance1 = ConditionManager.getInstance();
>
> Thread otherThread = new Thread() {
> public void run() {
> ConditionManager.resetInstance();
> }
> }.start();
>
> otherThread.join();
>
> assertEquals(instance1, ConditionManager.getInstance());
> }
thanks
you're right - it was easy :-)
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> Stefano Bagnara ha scritto:
>>> /**
>>> * 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) ?
>> Forgot to say that if this is the case it should be pretty easy to
>> create a test case to prove it.
>
> how would you approach the management of the multiple threads?
I'm writing the code in thunderbird, so forget syntax and other
problems. Hope you get the idea:
void testIssue() {
ConditionManager instance1 = ConditionManager.getInstance();
Thread otherThread = new Thread() {
public void run() {
ConditionManager.resetInstance();
}
}.start();
otherThread.join();
assertEquals(instance1, ConditionManager.getInstance());
}
otherwise, instead of checking the ConditionManager methods directly we
could check the call to the AbstractConditionalCommand.execute.
get the manager, get the condition manager instance, run the execute in
the main thread, run the execute of another thread using the same
pattern above, check the condition manager instance (it has to be the
same again).
Of course this simply prove this issue and does not check for every
synchronization error, but I find it useful to prove bugs with tests and
see them greened by the patch.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Stefano Bagnara ha scritto:
> > /**
> > * 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) ?
>
> Forgot to say that if this is the case it should be pretty easy to
> create a test case to prove it.
how would you approach the management of the multiple threads?
> Furthermore, I see that this block is allowed to send SieveException.
> Maybe the setInstance(oldManager) should be called in a finally to make
> sure we place back the status to the right one (well, maybe after an
> exception the ConditionManager is ignored, but I think it is anyway
> better to cleanup).
+1
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> Furthermore, I see that this block is allowed to send SieveException.
>> Maybe the setInstance(oldManager) should be called in a finally to make
>> sure we place back the status to the right one (well, maybe after an
>> exception the ConditionManager is ignored, but I think it is anyway
>> better to cleanup).
>
> http://svn.apache.org/repos/asf/james/jsieve/trunk/src/main/java/org/apache/jsieve/commands/AbstractConditionalCommand.java
>
> already has
>
> protected Object execute(MailAdapter mail, Block block) throws
> SieveException
> {
> // Switch to a new ConditionManager
> ConditionManager oldManager = ConditionManager.getInstance();
> ConditionManager.resetInstance();
>
> try
> {
> // Execute the Block
> Object result = block.execute(mail);
> return result;
> }
> finally
> {
> // Always restore the old ConditionManager
> ConditionManager.setInstance(oldManager);
> }
> }
>
> hope that this should cover it
>
> - robert
You are right! Sorry but I was examining an old local checkout of jsieve
and I didn't remember that you already fixed this more than one month
ago (this is the second diff I lost in that time frame... maybe I've had
a black out ;-) ).
The fix you already applied was exactly what I was proposing ;-)
Thank you,
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Stefano Bagnara ha scritto:
> > /**
> > * 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;
> > }
<snip>
> Furthermore, I see that this block is allowed to send SieveException.
> Maybe the setInstance(oldManager) should be called in a finally to make
> sure we place back the status to the right one (well, maybe after an
> exception the ConditionManager is ignored, but I think it is anyway
> better to cleanup).
http://svn.apache.org/repos/asf/james/jsieve/trunk/src/main/java/org/apache/jsieve/commands/AbstractConditionalCommand.java
already has
protected Object execute(MailAdapter mail, Block block) throws
SieveException
{
// Switch to a new ConditionManager
ConditionManager oldManager = ConditionManager.getInstance();
ConditionManager.resetInstance();
try
{
// Execute the Block
Object result = block.execute(mail);
return result;
}
finally
{
// Always restore the old ConditionManager
ConditionManager.setInstance(oldManager);
}
}
hope that this should cover it
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Stefano Bagnara ha scritto:
> /**
> * 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) ?
Forgot to say that if this is the case it should be pretty easy to
create a test case to prove it.
Furthermore, I see that this block is allowed to send SieveException.
Maybe the setInstance(oldManager) should be called in a finally to make
sure we place back the status to the right one (well, maybe after an
exception the ConditionManager is ignored, but I think it is anyway
better to cleanup).
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Bernd Fondermann <be...@googlemail.com>.
On 8/9/07, Robert Burrell Donkin <ro...@gmail.com> wrote:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> > Robert Burrell Donkin ha scritto:
> > > On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> > > https://issues.apache.org/jira/browse/JSIEVE-10 suggests removing the
> > > singletons and injecting the dependency instead. not sure how much
> > > work that is likely to be.
> > >
> > > - robert
> >
> > As a personal approach I try to avoid static fields (singletons, thread
> > locals and anything else using static fields) at all costs, but I know
> > I'm a bit extreme at this.
>
> same here
+1
and not extreme at all. this is considered a code smell broadly. for
example, it hinders proper unit testing.
Bernd
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Release near? (Was: Sporadic Mysterious Exceptions)
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> Robert Burrell Donkin ha scritto:
>
> <snip>
>
>>> i'd like a sieve release in time for the milestone. if these fixes
>>> work then it would be better to leave the refactoring till afterwards.
>>>
>>> - robert
>> This make sense. jSieve also deserve an official Apache release like we
>> did for mime4j. We have this project in svn since too long and it is
>> real pity there is no official releases.
>
> i think we're in a reasonable position to think about releasing a 0.2
> sometime soon
>
> i've created a couple of versions in JIRA:
>
> * 0.2 is for work i think i'll find time to do soon
> * 1.0 is for work i either think looks a bit big or risky for 0.2
>
> and assigned the remaining issues to them. if anyone wants to help
> then please jump in.
>
> - robert
Thank you! I feel really better when I can open the JIRA RoadMap page
and clearly understand what are the next releases and what are the
already scheduled issues to be fixed for a given release.
I agree on your choice to place JSIEVE-6 and JSIEVE-4 scheduled for 0.2
while delaying the others to a future release.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Release near? (Was: Sporadic Mysterious Exceptions)
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
<snip>
> > i'd like a sieve release in time for the milestone. if these fixes
> > work then it would be better to leave the refactoring till afterwards.
> >
> > - robert
>
> This make sense. jSieve also deserve an official Apache release like we
> did for mime4j. We have this project in svn since too long and it is
> real pity there is no official releases.
i think we're in a reasonable position to think about releasing a 0.2
sometime soon
i've created a couple of versions in JIRA:
* 0.2 is for work i think i'll find time to do soon
* 1.0 is for work i either think looks a bit big or risky for 0.2
and assigned the remaining issues to them. if anyone wants to help
then please jump in.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Release near? (Was: Sporadic Mysterious Exceptions)
Posted by Stefano Bagnara <ap...@bago.org>.
Stefano Bagnara ha scritto:
> jSieve also deserve an official Apache release like we
> did for mime4j. We have this project in svn since too long and it is
> real pity there is no official releases.
To avoid misunderstandings, this is not to hurry you on a release,
Robert! I just wanted to collect opinions on these minor patches (and I
see Robert already looked at JSIEVE-5 while I was writing this).
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
[JSieve] Release near? (Was: Sporadic Mysterious Exceptions)
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> Robert Burrell Donkin ha scritto:
>>> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>>> https://issues.apache.org/jira/browse/JSIEVE-10 suggests removing the
>>> singletons and injecting the dependency instead. not sure how much
>>> work that is likely to be.
>>>
>>> - robert
>> As a personal approach I try to avoid static fields (singletons, thread
>> locals and anything else using static fields) at all costs, but I know
>> I'm a bit extreme at this.
>
> same here
>
>> That said I don't have any knowledge on the jSieve architecture so I can
>> only give you my +1 to remove them, but I will not be able to help or to
>> estimate the complexity.
>
> i'd like a sieve release in time for the milestone. if these fixes
> work then it would be better to leave the refactoring till afterwards.
>
> - robert
This make sense. jSieve also deserve an official Apache release like we
did for mime4j. We have this project in svn since too long and it is
real pity there is no official releases.
As one of our known user (an Atlassian developer) added a feature (body
support) some month ago and attached it here:
http://issues.apache.org/jira/browse/JSIEVE-15
I'd like to see this included before a release.
Also http://issues.apache.org/jira/browse/JSIEVE-5 and
http://issues.apache.org/jira/browse/JSIEVE-6 seems to be easy fixes.
What do other thinks? Are the patches too risky or is it better to
include them before trying to cut a release?
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
> > On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> > https://issues.apache.org/jira/browse/JSIEVE-10 suggests removing the
> > singletons and injecting the dependency instead. not sure how much
> > work that is likely to be.
> >
> > - robert
>
> As a personal approach I try to avoid static fields (singletons, thread
> locals and anything else using static fields) at all costs, but I know
> I'm a bit extreme at this.
same here
> That said I don't have any knowledge on the jSieve architecture so I can
> only give you my +1 to remove them, but I will not be able to help or to
> estimate the complexity.
i'd like a sieve release in time for the milestone. if these fixes
work then it would be better to leave the refactoring till afterwards.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> So, was this simply a test for us?? You already found that.. was it a
>> secret?? :-P
>
> i only found one problem: you and bernd found another three. i'd much
> rather do a comprehensive job than quickly commit a partial fix.
I was joking :-)
Great team work! This is what I like to see! (and I like it even more
when I have simply to look some source file and you do the commits and
test it :-) ).
>> Next time just share also the solutions and not only the problem :-P
>
> i'm not convinced that it's really working correctly. the other areas
> need fixing as well or they're just going to break again.
>
>> Let's hope at least it really works properly :-)
>
> it's just in my local fork. would prefer a more comprehensive solution
>
> https://issues.apache.org/jira/browse/JSIEVE-10 suggests removing the
> singletons and injecting the dependency instead. not sure how much
> work that is likely to be.
>
> - robert
As a personal approach I try to avoid static fields (singletons, thread
locals and anything else using static fields) at all costs, but I know
I'm a bit extreme at this.
That said I don't have any knowledge on the jSieve architecture so I can
only give you my +1 to remove them, but I will not be able to help or to
estimate the complexity.
Thank you,
Stefano
PS: it is cool to see you active on mime4j and jsieve. They really
needed some more care after so many years alone.
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
> Robert Burrell Donkin ha scritto:
> > On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
<snip>
> >> 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
>
> So, was this simply a test for us?? You already found that.. was it a
> secret?? :-P
i only found one problem: you and bernd found another three. i'd much
rather do a comprehensive job than quickly commit a partial fix.
> Next time just share also the solutions and not only the problem :-P
i'm not convinced that it's really working correctly. the other areas
need fixing as well or they're just going to break again.
> Let's hope at least it really works properly :-)
it's just in my local fork. would prefer a more comprehensive solution
https://issues.apache.org/jira/browse/JSIEVE-10 suggests removing the
singletons and injecting the dependency instead. not sure how much
work that is likely to be.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> wrote:
>> 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
That's exactly what I was thinking right now.
Other code simply call the getInstance. The reset and the set are only
called there, so a stack seems much more appropriate.
>> 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
So, was this simply a test for us?? You already found that.. was it a
secret?? :-P
Next time just share also the solutions and not only the problem :-P
Let's hope at least it really works properly :-)
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@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
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
Robert Burrell Donkin ha scritto:
> On 8/9/07, Stefano Bagnara <ap...@bago.org> 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!!
>
> +1
>
> not sure that it's possible to create a good test for this but
> https://issues.apache.org/jira/browse/JSIEVE-10 demonstrates that it
> occurs in the wild. unless someone jumps in with a good idea for a
> test, i'll commit a fix without a test.
>
> - robert
Go ahead: I don't think a test can be written for similar racing
conditions as there is no way to control 2 threads in order to have them
executing that code with the specific timing needed to show the problem.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Robert Burrell Donkin <ro...@gmail.com>.
On 8/9/07, Stefano Bagnara <ap...@bago.org> 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!!
+1
not sure that it's possible to create a good test for this but
https://issues.apache.org/jira/browse/JSIEVE-10 demonstrates that it
occurs in the wild. unless someone jumps in with a good idea for a
test, i'll commit a fix without a test.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Bernd Fondermann <be...@googlemail.com>.
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 <ap...@bago.org> 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 <ro...@gmail.com> 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: server-dev-unsubscribe@james.apache.org
> >> For additional commands, e-mail: server-dev-help@james.apache.org
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> > For additional commands, e-mail: server-dev-help@james.apache.org
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
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 <ro...@gmail.com> 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: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Bernd Fondermann <be...@googlemail.com>.
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.
But maybe this case will never occur because of the special nature of
ThreadLocals?
Bernd
On 8/6/07, Robert Burrell Donkin <ro...@gmail.com> 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: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Stefano Bagnara <ap...@bago.org>.
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: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: [JSieve] Sporadic Mysterious Exceptions
Posted by Serge Knystautas <sk...@gmail.com>.
On 8/6/07, Robert Burrell Donkin <ro...@gmail.com> wrote:
http://svn.apache.org/repos/asf/james/jsieve/trunk/src/main/java/org/apache/jsieve/commands/CommandStateManager.java.
> 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.
+1. Yeah, looking at that code it looks like just making a
ThreadLocal subclass would be much more straight-forward. I'm not
sure how this pattern got used and if there is something else to this.
--
Serge Knystautas
Lokitech >> software . strategy . design >> http://www.lokitech.com
p. 301.656.5501
e. sergek@lokitech.com
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org