Return-Path: 
 <dev-return-111577-apmail-commons-dev-archive=commons.apache.org@commons.apache.org>
Delivered-To: apmail-commons-dev-archive@www.apache.org
Received: (qmail 77974 invoked from network); 22 Nov 2008 17:14:09 -0000
Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2)
  by minotaur.apache.org with SMTP; 22 Nov 2008 17:14:09 -0000
Received: (qmail 72699 invoked by uid 500); 22 Nov 2008 17:14:17 -0000
Delivered-To: apmail-commons-dev-archive@commons.apache.org
Received: (qmail 72189 invoked by uid 500); 22 Nov 2008 17:14:16 -0000
Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@commons.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@commons.apache.org>
List-Post: <mailto:dev@commons.apache.org>
List-Id: <dev.commons.apache.org>
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Delivered-To: mailing list dev@commons.apache.org
Received: (qmail 72178 invoked by uid 99); 22 Nov 2008 17:14:16 -0000
Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136)
    by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 22 Nov 2008 09:14:16 -0800
X-ASF-Spam-Status: No, hits=-0.0 required=10.0
	tests=SPF_PASS
X-Spam-Check-By: apache.org
Received-SPF: pass (athena.apache.org: domain of sebbaz@gmail.com designates
 66.249.92.169 as permitted sender)
Received: from [66.249.92.169] (HELO ug-out-1314.google.com) (66.249.92.169)
    by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 22 Nov 2008 17:12:50 +0000
Received: by ug-out-1314.google.com with SMTP id j40so415313ugd.26
        for <dev@commons.apache.org>; Sat, 22 Nov 2008 09:13:25 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=gamma;
        h=domainkey-signature:received:received:message-id:date:from:to
         :subject:in-reply-to:mime-version:content-type
         :content-transfer-encoding:content-disposition:references;
        bh=wtEJOhhk/lc9QwK2MpehKw68UKKKplaks813I6pjemE=;
        b=oweV25wmRkUX1k3yIZDDLhwzAYmxGG85AWb25byxLA75wdxJBp3bE6MiHjWKhXXe/m
         4SS0k/yS9PXADhSCzClFp6TvkOiR/5CqfCtmTT59pHqVuT0zR8FltzaGF+mGcsZ7QaOF
         HlL1tQN7ixdYrXx8cRto7wcOg50J/Et6cXX10=
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:date:from:to:subject:in-reply-to:mime-version
         :content-type:content-transfer-encoding:content-disposition
         :references;
        b=LBQ6jc+1JUo8N8TUBFlYlZ1P6+kEv0pPZeerIEe7MnM2jY42xlpQiAQsri9OWjqGzX
         jZtstb+gzaQnZu2/5rUkTfTc3a00Q4TZK06asbPAsmYaYfipK5KGhYh2GgIs9mbLlGJx
         S+nTK5p0RH+A0kfsPDxFiaw3JN+Rzrf4GwjME=
Received: by 10.86.95.8 with SMTP id s8mr1240852fgb.79.1227374004935;
        Sat, 22 Nov 2008 09:13:24 -0800 (PST)
Received: by 10.86.65.7 with HTTP; Sat, 22 Nov 2008 09:13:24 -0800 (PST)
Message-ID: <25aac9fc0811220913w58892738x67e991b2c5926430@mail.gmail.com>
Date: Sat, 22 Nov 2008 17:13:24 +0000
From: sebb <sebbaz@gmail.com>
To: "Commons Developers List" <dev@commons.apache.org>
Subject: Re: [all] Commons SCXML 0.9 RC1 available
In-Reply-To: <ce1f2ea80811220806g7535c49k407e3608d076a482@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
References: <ce1f2ea80811211751l34849a1eu98e7e527a7dc7db7@mail.gmail.com>
	 <25aac9fc0811211814y384424f8ra6f4863908cedb37@mail.gmail.com>
	 <ce1f2ea80811211815u3b46ace9j75816d418228a251@mail.gmail.com>
	 <25aac9fc0811211851h5dec55c6jc36570d92d39c5b0@mail.gmail.com>
	 <ce1f2ea80811211920o5c33604dkbbae84bf9f613b32@mail.gmail.com>
	 <25aac9fc0811220546ycce3926l24f8e5f108a47e9b@mail.gmail.com>
	 <25aac9fc0811220619g44c778bbjbf5d4683af9e5155@mail.gmail.com>
	 <ce1f2ea80811220806g7535c49k407e3608d076a482@mail.gmail.com>
X-Virus-Checked: Checked by ClamAV on apache.org

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=E9j=
=E0
>  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 serialisabl=
e?
If not, then it's an easy fix to use private static final instead of privat=
e.
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 =3D 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.
>  >
>  > =3D=3D
>  >
>  > org.apache.commons.scxml.io.SCXMLDigester.digest() has:
>  >        if (scxml !=3D null) {
>  >            ModelUpdater.updateSCXML(scxml);
>  >        }
>  >        scxml.setLegacy(true); // could be null - should be in
>  > previous if statement
>  >
>  > =3D=3D
>
> <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.

>  > =3D=3D
>  >
>  > 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+?

>  > =3D=3D
>  >
>  > Inconsistent synchronization of
>  > org.apache.commons.scxml.SCXMLExecutor.errorReporter; locked 76% of
>  > time
>  >
>  > Similarly for eventDispatcher, stateMachine, superStep in the same cla=
ss
>  >
>  > 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.
>  >
>  > =3D=3D
>  >
>  > 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).
>  >
>  > =3D=3D
>  >
>  > 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