Mailing-List: contact dev-help@geronimo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geronimo.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <452E54BD-45BE-4206-8CA9-5A622995BE53@yahoo.com>
References: <1f3854d50910221327o42bd3557t62acc5444506d7c1@mail.gmail.com>
	<06F9F00B-AB33-4A8B-8ECE-85D2BECC3E19@visi.com>
 <452E54BD-45BE-4206-8CA9-5A622995BE53@yahoo.com>
From: Quintin Beukes <quintin@skywalk.co.za>
Date: Sun, 25 Oct 2009 14:44:53 +0200
Message-ID: <1f3854d50910250544m67c08998rfebd7e86ed69e28@mail.gmail.com>
Subject: Re: Singletons in Geronimo
To: dev@geronimo.apache.org
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hey,

Thanks for the analysis. Would be great to not have to rerelease
OpenEJB. I will have a look at doing anything needed to get it running
with the new modifications.

Finally, there is something else in the patches (other than the first
deadlock). When you invoke on a Singleton, itcauses the ReadWriteLock
to fail with a NullPointerException. It's related to the AccessTimeout
option for the SingletonContainer. If it doesn't specify a unit, then
the Duration object parses a null "unit". This causes the Unit object
in SingletonContainer to be set to null. I included in the patch a
check for null, which then causes a default of TimeUnit.SECONDS to be
used.

I think it's a good idea to modify OpenEJB for this in any case.
Unless you guys want a unit specified as a requirement? This might be
a bit counter intuitive, as one is used to simply specifying a number
for a timeout. And as I understand the source code for
Duration.parse(), the unit was made to be optional. So if this was the
intention, such a modification would be needed somewhere in OpenEJB.
OR in the GBean a unit could be "injected" into the timeout value. In
such a case problems could still occur when these values are
configured in a pure OpenEJB environment.

Though to avoid another release at this point, I'll simply add a unit
in Geronimo's config.xml.

So I'll make 2 patch sets. One for Geronimo at the point. And another
for OpenEJB's trunk. Let me know what you guys choose to do regarding
this unit.

Quintin Beukes


On Sat, Oct 24, 2009 at 3:29 AM, David Jencks <david_jencks@yahoo.com> wrot=
e:
> I attached a patch to=C2=A0GERONIMO-4918=C2=A0that mostly reverses the de=
pendencies
> between EjbModuleImpl and EjbDeployment so the deployments will start fir=
st.
> =C2=A0I think this is the main bit needed for David's idea on how to fix =
this
> without the wait() code in the geronimo wiring. =C2=A0There are some othe=
r
> changes needed before this will work such as removing the lifecycle metho=
ds
> from EjbDeployment. =C2=A0It also annotates EjbModuleImpl and adds wildca=
rd
> support to collection valued references, I may well commit this last bit
> separately.
> thanks
> david jencks
> On Oct 23, 2009, at 5:33 PM, David Blevins wrote:
>
> On Oct 22, 2009, at 1:27 PM, Quintin Beukes wrote:
>
> Unfortunately the only way I could see how to get the @Startup working
>
> was to modify OpenEJB to create a property which gives the
>
> responsibility over to Geronimo, and there the only way was to create
>
> a new GBean which has it's lifecycle doStart() called after all EJBs
>
> are in the RUNNING state.
>
> I couldn't find a better way.
>
> Very impressive that you could find the problem at all as well as a worka=
ble
> fix. =C2=A0That code makes my brain hurt nearly every time I look at it.
>
> I checked in the more generic Singleton code. =C2=A0Left out the alternat=
e
> startup code -- though it was actually pretty clever.
>
> This chunk of code in GeronimoThreadContextListener was not there when we
> wrote the initial integration, so I went digging around as to why it was
> added (there shouldn't be any locking code in the integration at all, muc=
h
> less wait/notify):
>
> At the 10,000 foot view, this chunk of code in GeronimoThreadContextListe=
ner
> must die:
>
> =C2=A0=C2=A0=C2=A0synchronized (deploymentInfo) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0if (deploymentInfo.get(EjbDeployment.class) =3D=3D null) =
{
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!deploymentInfo.isDestroyed()=
) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0try {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0deploymentInfo.wait();
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} catch (=
InterruptedException e) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0log.warn(=
"Wait on deploymentInfo interrupted
> unexpectedly");
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0}
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>
> Seems that code was added for GERONIMO-3780 which is essentially the same=
 as
> the Singleton injection/lookup issue. =C2=A0Both issues boil down to the =
fact
> that lookups may happen inside the EjbModuleImpl.start() method where ejb=
s
> are created by OpenEJB. =C2=A0The wait/notify block works for MDBs as all=
 their
> lookups will happen in other threads and not inside the startup thread.
> =C2=A0With Singletons this is not the case, so the startup thread ends up=
 waiting
> on its own thread and a deadlock occurs.
>
> Ultimately, this is flawed and the data that is required in
> GeronimoThreadContextListener needs to be made available in some way befo=
re
> we call EjbModuleImpl.start(). =C2=A0I talked it over with David Jencks a=
nd the
> EjbDeployment objects are available when the EjbModuleImpl gbean is start=
.
> =C2=A0Then we should be able to hand them directly to the
> GeronimoThreadContextLister *before* asking OpenEJB to create the EJBs (a=
nd
> subsequently do lookups). =C2=A0When the contextEntered method is called =
we can
> complete any initialization as at that point we will have the
> CoreDeploymentInfo object and can hook it up with the EjbDeployment objec=
t.
>
> So we should be able to get a solution in there that removes all locking
> code, works for singletons and mdbs, and doesn't require a new OpenEJB
> release. =C2=A0Finger's crossed anyway :)
>
> Thanks for all the work bringing it this far. =C2=A0Really you did all th=
e hard
> work. =C2=A0Very *very* appreciated.
>
> -David
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> [1] =C2=A0http://issues.apache.org/jira/browse/GERONIMO-3780
>
>