commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [all] Commons SCXML 0.9 RC1 available
Date Sat, 22 Nov 2008 17:13:24 GMT
On 22/11/2008, Rahul Akolkar <rahul.akolkar@gmail.com> wrote:
> On Sat, Nov 22, 2008 at 9:19 AM, sebb <sebbaz@gmail.com> wrote:
>  > Just thought to run Findbugs on the code.
>  >
>
> <snip/>
>
>  Thanks for going over the Findbugs list. While this is a bit of a déjà
>  vu for me ...
>
>     http://markmail.org/message/si5kphud52ntxbqi
>
>  ... I don't expect others to remember the discussion :-) and should
>  have commented on it earlier in this thread.
>

I've just had a look, and it raises some issues:

Are all common Log implementations serialisable?
Does it make sense for Log implementations to have to implement serialisable?
If not, then it's an easy fix to use private static final instead of private.
Alternatively, one could document that the code assumes the logging
implementation is serialisable.

The discussion on threading mentions setting variables once, and
referring to them later, both without synchronisation. This is not
guaranteed to work, unless the thread that sets the data then uses a
lock that is also used by the reading thread(s) at least once after
the variable was set. [With a shared lock, changes to shared variables
by thread A "happen before" the lock is released, and lock release
"happens before" the lock is acquired by thread B.]

Constructors don't guarantee safe publishing either. Non-final
instance fields that are set in a constructor and never updated are
not necessarily accessible to other threads - that is why the
java.lang.String instance fields had to be made final in Java 1.5. It
is a good idea to do the same so as far as possible in other code.

It's unlikely that testing - or even much production use - will ever
find such problems, but that does not mean they cannot happen.

It would be nice if the thread-safety guarantees (or otherwise) were
documented in the Javadoc. Maybe that is something to work towards for
the next release.

>  There is one new finding, I'll comment on it below.
>
>
>
>  > There are a lot of cases of the statement:
>  >
>  > private Log appLog = LogFactory.getLog(...)
>  >
>  > which appear in serializable classes.
>  >
>  > However Log does not appear to be Serializable, so this will cause a
>  > problem if the classes are serialised. So long as the Log fields are
>  > private, they could just be made static and final.
>  >
>  > ==
>  >
>  > org.apache.commons.scxml.io.SCXMLDigester.digest() has:
>  >        if (scxml != null) {
>  >            ModelUpdater.updateSCXML(scxml);
>  >        }
>  >        scxml.setLegacy(true); // could be null - should be in
>  > previous if statement
>  >
>  > ==
>
> <snap/>
>
>  Indeed :-) I'll fix that in a minute, but I won't be cutting a new RC
>  just for this:
>  1) We could do better than a NPE, but its a fatal parsing error at that point
>  2) Its a long deprecated class (deprecated in 0.7)

OK, no problem with that.

>  -Rahul
>
>
>
>
>  >
>  > Class org.apache.commons.scxml.model.Data defines non-transient
>  > non-serializable instance field node
>  >

I've had a look at a few of the classes that implement Node, and not
yet found one that implements serialisable, so I don't know how that's
going to work.

>  > ==
>  >
>  > org.apache.commons.scxml.env.jexl.JexlEvaluator.evalCond(Context,
>  > String) has Boolean return type and returns explicit null
>  >
>  > org.apache.commons.scxml.env.jsp.ELEvaluator.evalCond(Context, String)
>  > has Boolean return type and returns explicit null
>  >
>  > These will cause problems if used later with autoboxing.

Are these not problems when using Java 1.5+?

>  > ==
>  >
>  > Inconsistent synchronization of
>  > org.apache.commons.scxml.SCXMLExecutor.errorReporter; locked 76% of
>  > time
>  >
>  > Similarly for eventDispatcher, stateMachine, superStep in the same class
>  >
>  > It appears that the class will be used in multiple threads, so if
>  > those variables can be accessed from multiple threads, then all access
>  > to them will need to be synchronized.
>  >
>  > ==
>  >
>  > There are a few other Findbugs warnings - mainly in the test code. Let
>  > me know if you want the full list (though if you use Eclipse, it's
>  > probably easier to add the Findbugs plugin).
>  >
>  > ==
>  >
>  > I also just noticed that there are a few instance fields that could be
>  > made final - e.g. most of the items set up in the
>  > org.apache.commons.scxml.SCInstance.SCInstance constructor.
>  > This would improve thread-safety.
>  >
>
>
> ---------------------------------------------------------------------
>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  For additional commands, e-mail: dev-help@commons.apache.org
>
>

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


Mime
View raw message