logic around cas reset for cas multiplier CASes

[Marshall Schor <ms...@schor.com>]

While working on a fix for UIMA-2211, I came across code in the core that looks
wrong.

There is code for getEmptyCas in UimaContext_ImplBase, that had comments to the
effect that when the empty CAS was provided back to user code that requested it
(typically within a Cas Multiplier), the Cas was "locked" (meaning that you
cannot reset it) and any class loader switching that might be needed for loading
things related to the Cas (such as JCas cover classes) was done.  The comment
said that when the "next()" method returned to the framework, the Cas would be
unlocked (and the class loaders restored).

The scenario I think makes sense.  The user code typically implements a "next()"
method, called by the framework, to return another CAS, so the first thing that
method does is get an empty one, then do some initialization, and the return
it.  So while in the user code, the CAS should have the right class loaders set
up for it, and be "locked" against operations that the user code shouldn't be doing.

However, even though the comments say this, the actual code that was there
called SwitchClassLoader (which skips locking the CAS) instead of
switchClassLoaderLockCasCL (which does the lock).  So I think this should be fixed.

I changed it, and found some tests didn't run.  One I traced to the code in
CasPool for releasing a CAS.  It has code to unlock and restore the class
loaders, and reset the CAS, but the order of these were backwards in the method
releaseCas(...) - - it was trying to reset the Cas before unlocking it...).

Another testcase which now fails is testEnableReset in CasManager_implTest. 
This test says that if a Cas is locked, you should not be able to release it
back to the CasPool, but now that I fixed the ordering, this does work.  So I
think the test is wrong to expect releasing it to fail.

-----------

Before I commit things, does anyone know of use cases where getEmptyCas should
*not* lock a CAS (meaning prevent users from calling reset on the cas?

Thanks for thinking about this :-)  -Marshall

Re: logic around cas reset for cas multiplier CASes

[Marshall Schor <ms...@schor.com>]

Further (overnight) thinking on this issue:

Locking the cas that getEmptyCas returns may break some existing user code.

And there's no need to protect users at this point against the actions that
locking prevents, I think.

So, I'm changing my approach - to keep getEmptyCas as it is - returning an
unlocked CAS.
However, I will put in the other change for releasing a CAS to unlock it before
resetting it.
I don't see any downside to that.

-Marshall

On 8/11/2011 5:56 PM, Marshall Schor wrote:
> While working on a fix for UIMA-2211, I came across code in the core that looks
> wrong.
>
> There is code for getEmptyCas in UimaContext_ImplBase, that had comments to the
> effect that when the empty CAS was provided back to user code that requested it
> (typically within a Cas Multiplier), the Cas was "locked" (meaning that you
> cannot reset it) and any class loader switching that might be needed for loading
> things related to the Cas (such as JCas cover classes) was done.  The comment
> said that when the "next()" method returned to the framework, the Cas would be
> unlocked (and the class loaders restored).
>
> The scenario I think makes sense.  The user code typically implements a "next()"
> method, called by the framework, to return another CAS, so the first thing that
> method does is get an empty one, then do some initialization, and the return
> it.  So while in the user code, the CAS should have the right class loaders set
> up for it, and be "locked" against operations that the user code shouldn't be doing.
>
> However, even though the comments say this, the actual code that was there
> called SwitchClassLoader (which skips locking the CAS) instead of
> switchClassLoaderLockCasCL (which does the lock).  So I think this should be fixed.
>
> I changed it, and found some tests didn't run.  One I traced to the code in
> CasPool for releasing a CAS.  It has code to unlock and restore the class
> loaders, and reset the CAS, but the order of these were backwards in the method
> releaseCas(...) - - it was trying to reset the Cas before unlocking it...).
>
> Another testcase which now fails is testEnableReset in CasManager_implTest. 
> This test says that if a Cas is locked, you should not be able to release it
> back to the CasPool, but now that I fixed the ordering, this does work.  So I
> think the test is wrong to expect releasing it to fail.
>
> -----------
>
> Before I commit things, does anyone know of use cases where getEmptyCas should
> *not* lock a CAS (meaning prevent users from calling reset on the cas?
>
> Thanks for thinking about this :-)  -Marshall
>
>