commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rahul Akolkar" <rahul.akol...@gmail.com>
Subject Re: [all] Commons SCXML 0.9 RC1 available
Date Sat, 22 Nov 2008 16:06:57 GMT
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.

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)

-Rahul



>
> Class org.apache.commons.scxml.model.Data defines non-transient
> non-serializable instance field node
>
> ==
>
> 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.
>
> ==
>
> 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


Mime
View raw message