[shale][clay] Serializability (was Re: [shale] Import statements)

[Rahul Akolkar <ra...@gmail.com>]

On 11/15/05, Craig McClanahan <cr...@apache.org> wrote:
> On 11/14/05, Rahul Akolkar <ra...@gmail.com> wrote:
<snip/>
> >
> > The other major item its complaining about is non-transient
> > non-serializable fields in Serializable classes. That explains some of
> > the NotSerializableException's I get with sticky sessions on Tomcat
> > startup (mostly in subclasses of Commons Chain's base impls). Will
> > take a stab at that soon.
>
>
> Thanks ... those are important. Not just because we need to honor the
> contracts about Serializable session scope attributes, but they also
> indicate potential architectural issues if we are maintaining references to
> non-Serializable stuff in session scope that should really not be
> referenced. Struts 1.x has suffered from some of this kind of thing, causing
> memory bloat due to duplicated object trees when you migrate a session from
> one instance to another.
>
<snap/>

I got around to looking at some code today in serializability's light,
and that leads to a few questions. I bring this up because I've err'ed
here before and I wouldn't want [shale] (or [resources] - a similar
discussion is probably in order on commons-dev) to do the same.

First, the offenders include:

[shale]
 * ServletRemoteContext
 * CommonsValidatorTag
 * CommonsValidator
 * ShalePhaseListener
 * ShaleWebContext
 * StatusImpl

[clay]
 * ClayContext
 * ComponentBean
 * BuilderRuleContext
 * SymbolTag

Some might be serializable, others are clearly not, even with any lazy
initialization attempts. So, for each, maybe its best to revisit what
prompted the serializability requirement, and how much each needs to
be pushed.

My cursory observations:
 * For the *Tag impls there isn't much leeway, so it might be worth
biting the bullet
 * StatusImpl needs to be serializable by design
 * ShaleWebContext is in a tough spot already (see below)

What are the Shale developers' views on the rest?

It might not be a bad idea to have test cases to show that the round
trips work as advertised (I can volunteer some), once this gets sorted
out.


> For the cases where this is really issues with Commons Chain implementation
> classes, we will (of course) need to supply patches for that. Fortunately,
> I'm a committer there too :-).
>
<snip/>

Yes, having looked at the developer list for [chain], let me ask one
[OT] question here, and I'll move to commons-dev (or user) if it
becomes apparent this deserves its own thread:

Digging into [chain], I found a related commit message on top of
ContextBase [1]. Were there any further developments on that? Seems
like it may be required to allow subclass implementors (of the base
impls in chain) to make their own decisions about serializability to
liberate the *WebContext classes, but that probably won't fit a minor
release (I'm not even sure if one is planned for [chain]).

-Rahul

[1] http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/chain/trunk/src/java/org/apache/commons/chain/impl/ContextBase.java

>
> -Rahul
>
>
> Craig
>

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

Re: [shale][clay] Serializability (was Re: [shale] Import statements)

[Mike Kienenberger <mk...@gmail.com>]

On 11/29/05, Craig McClanahan <cr...@apache.org> wrote:
> * CommonsValidator
>
> This is a JSF Validator, so it could well be attached to a UIComponent
> stored in session scope.  It's not obvious from a static review of the code
> what's the problem, but if there is one it'll need to be fixed.

It should either implement StateHolder or Serializable.   In JSF 1.2
and in facelets (1.1 or 1.2), validators are added to the component
tree rather than recreated each request.

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

Re: [shale][clay] Serializability (was Re: [shale] Import statements)

[Rahul Akolkar <ra...@gmail.com>]

On 11/29/05, Craig McClanahan <cr...@apache.org> wrote:
> On 11/29/05, Rahul Akolkar <ra...@gmail.com> wrote:
> >
<snip/>
> In general, here's the policy that I propose to follow w.r.t. serialization:
>
> * Any class for which an instance might get stored (by the framework,
>  or by the application with the blessing of framework documentation
>  that says to do so) must be serializable.
>
> * Any class that is not serializable should not claim that it
>  "implements Serializable".
>
<snap/>

Indeed.


> That being said, let's review the cases:
>
<snip/>
>
> * CommonsValidator
>
>
> This is a JSF Validator, so it could well be attached to a UIComponent
> stored in session scope.  It's not obvious from a static review of the code
> what's the problem, but if there is one it'll need to be fixed.
>
<snap/>
> * StatusImpl
>
>
> Absolutely has to be serializable -- it's probably the non-static
> non-transient Log instance here.
>
<snip/>

Actually, both classes have declared fields that are non-serializable
interface types, rather than the serializable implementations of those
interfaces (this is probably a candidate to deviate from the Java best
practice of "coding to an interface" given the fields are private
implementation details hidden from the class' public contract). And
the logs.

I'll take a stab at these two in a few days, since there is already
consensus about the need to resolve these, and earlier this evening, I
told Niall I'd look at similar issues in [resources] anyway.


>
> > Digging into [chain], I found a related commit message on top of
> > ContextBase [1]. Were there any further developments on that? Seems
> > like it may be required to allow subclass implementors (of the base
> > impls in chain) to make their own decisions about serializability to
> > liberate the *WebContext classes, but that probably won't fit a minor
> > release (I'm not even sure if one is planned for [chain]).
>
>
> It's not obvious what we should do here ... but it's also not a decision we
> (Shale developers) or even we (Struts developers) can make in a vacuum.
> Needs to be addressed on commons-dev so any other stakeholders who care can
> pitch in an opinion.
>
<snip/>

Will move this bit to commons-dev when I get a chance.

-Rahul


>
> -Rahul
>
>
> Craig
>
>
> [1]
> > http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/chain/trunk/src/java/org/apache/commons/chain/impl/ContextBase.java
> >

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

Re: [shale][clay] Serializability (was Re: [shale] Import statements)

[Craig McClanahan <cr...@apache.org>]

On 11/29/05, Rahul Akolkar <ra...@gmail.com> wrote:
>
> On 11/15/05, Craig McClanahan <cr...@apache.org> wrote:
> > On 11/14/05, Rahul Akolkar <ra...@gmail.com> wrote:
> <snip/>
> > >
> > > The other major item its complaining about is non-transient
> > > non-serializable fields in Serializable classes. That explains some of
> > > the NotSerializableException's I get with sticky sessions on Tomcat
> > > startup (mostly in subclasses of Commons Chain's base impls). Will
> > > take a stab at that soon.
> >
> >
> > Thanks ... those are important. Not just because we need to honor the
> > contracts about Serializable session scope attributes, but they also
> > indicate potential architectural issues if we are maintaining references
> to
> > non-Serializable stuff in session scope that should really not be
> > referenced. Struts 1.x has suffered from some of this kind of thing,
> causing
> > memory bloat due to duplicated object trees when you migrate a session
> from
> > one instance to another.
> >
> <snap/>
>
> I got around to looking at some code today in serializability's light,
> and that leads to a few questions. I bring this up because I've err'ed
> here before and I wouldn't want [shale] (or [resources] - a similar
> discussion is probably in order on commons-dev) to do the same.


In general, here's the policy that I propose to follow w.r.t. serialization:

* Any class for which an instance might get stored (by the framework,
  or by the application with the blessing of framework documentation
  that says to do so) must be serializable.

* Any class that is not serializable should not claim that it
  "implements Serializable".

That being said, let's review the cases:

First, the offenders include:
>
> [shale]
> * ServletRemoteContext


Only used in the context of a single request.  Unfortunately, there is no
way to say "I'm not Serializabl even though my parent class claims that I
am" ... as is the case here, inherited from Commons chain.

* CommonsValidatorTag


Tag instances are used only in the context of a particular request -- no
reason to worry about serializability.

* CommonsValidator


This is a JSF Validator, so it could well be attached to a UIComponent
stored in session scope.  It's not obvious from a static review of the code
what's the problem, but if there is one it'll need to be fixed.


> * ShalePhaseListener


Never stored in session scope, so doesn't need to be serializable.

* ShaleWebContext


Only used in the context of a single request.  Same "inherited claim" issue
as ServletRemoteContext above.

* StatusImpl


Absolutely has to be serializable -- it's probably the non-static
non-transient Log instance here.

[clay]
> * ClayContext
> * ComponentBean
> * BuilderRuleContext
> * SymbolTag


I don't *think* any of these would ever need to be stored in session scope,
but Gary will need to confirm that.

Some might be serializable, others are clearly not, even with any lazy
> initialization attempts. So, for each, maybe its best to revisit what
> prompted the serializability requirement, and how much each needs to
> be pushed.
>
> My cursory observations:
> * For the *Tag impls there isn't much leeway, so it might be worth
> biting the bullet
> * StatusImpl needs to be serializable by design
> * ShaleWebContext is in a tough spot already (see below)
>
> What are the Shale developers' views on the rest?


See above.

It might not be a bad idea to have test cases to show that the round
> trips work as advertised (I can volunteer some), once this gets sorted
> out.


+1.

In similar scenarios for "day job" projects, we test serializability by
JUnit test cases that try to  serialize and then deserialize instances that
have been configured with various combinations of properties.  Not
foolproof, but certainly catches the obvious cases.

> For the cases where this is really issues with Commons Chain
> implementation
> > classes, we will (of course) need to supply patches for that.
> Fortunately,
> > I'm a committer there too :-).
> >
> <snip/>
>
> Yes, having looked at the developer list for [chain], let me ask one
> [OT] question here, and I'll move to commons-dev (or user) if it
> becomes apparent this deserves its own thread:
>
> Digging into [chain], I found a related commit message on top of
> ContextBase [1]. Were there any further developments on that? Seems
> like it may be required to allow subclass implementors (of the base
> impls in chain) to make their own decisions about serializability to
> liberate the *WebContext classes, but that probably won't fit a minor
> release (I'm not even sure if one is planned for [chain]).


It's not obvious what we should do here ... but it's also not a decision we
(Shale developers) or even we (Struts developers) can make in a vacuum.
Needs to be addressed on commons-dev so any other stakeholders who care can
pitch in an opinion.

My other general comment is that right at the moment I want to file an issue
against the Shale classes above that we know need to be addressed, then flag
it to be resolved LATER (and note in the release notes) so that we can
address it after the initial milestone release.

-Rahul


Craig


[1]
> http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/chain/trunk/src/java/org/apache/commons/chain/impl/ContextBase.java
>
> >
> > -Rahul
> >
> >
> > Craig
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>