Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Received-SPF: pass (nike.apache.org: domain of min.chen@citrix.com designates
 66.165.176.89 as permitted sender)
From: Min Chen <min.chen@citrix.com>
To: Darren Shepherd <darren.s.shepherd@gmail.com>, "dev@cloudstack.apache.org"
	<dev@cloudstack.apache.org>
Subject: Re: race conditions in VolumeServiceImpl.createBaseImageAsync()
 creates NPE
Thread-Topic: race conditions in VolumeServiceImpl.createBaseImageAsync()
 creates NPE
Thread-Index: AQHO1mij7SnQ8ioHjUmf2e6XfIzcPJoPhg2A
Date: Fri, 1 Nov 2013 00:29:34 +0000
Message-ID: <CE9843A4.4D6CE%min.chen@citrix.com>
In-Reply-To: 
 <CAMCEDC0FpovAHr_4=96P9D2gZJw8zcWJajgh==MET-6ee+wOwA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
user-agent: Microsoft-MacOutlook/14.3.6.130613
Content-Type: multipart/alternative;
	boundary="_000_CE9843A44D6CEminchencitrixcom_"
MIME-Version: 1.0

--_000_CE9843A44D6CEminchencitrixcom_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Darren, I just checked the code, you are right. In case of one thread throw=
s exception in downloading template to primary, it will delete the entry in=
 template_store_ref, causing the second thread failing with NPE. We need to=
 fix this in 4.3. Please file a bug for this.

Thanks
-min

From: Darren Shepherd <darren.s.shepherd@gmail.com<mailto:darren.s.shepherd=
@gmail.com>>
Date: Thursday, October 31, 2013 11:39 AM
To: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@clou=
dstack.apache.org<mailto:dev@cloudstack.apache.org>>, Min Chen <min.chen@ci=
trix.com<mailto:min.chen@citrix.com>>
Subject: race conditions in VolumeServiceImpl.createBaseImageAsync() create=
s NPE

The following code results in a NPE in bad situations

        templatePoolRef =3D _tmpltPoolDao.acquireInLockTable(templatePoolRe=
fId, storagePoolMaxWaitSeconds);

        if (templatePoolRef =3D=3D null) {
            if (s_logger.isDebugEnabled()) {
                s_logger.info<http://s_logger.info>("Unable to acquire lock=
 on VMTemplateStoragePool " + templatePoolRefId);
            }
            templatePoolRef =3D _tmpltPoolDao.findByPoolTemplate(dataStore.=
getId(), template.getId());
            if (templatePoolRef.getState() =3D=3D ObjectInDataStoreStateMac=
hine.State.Ready ) {
                s_logger.info<http://s_logger.info>("Unable to acquire lock=
 on VMTemplateStoragePool " + templatePoolRefId + ", But Template " + templ=
ate.getUniqueName() + " is already copied to primary storage, skip copying"=
);
                createVolumeFromBaseImageAsync(volume, templateOnPrimarySto=
reObj, dataStore, future);
                return;
            }
            throw new CloudRuntimeException("Unable to acquire lock on VMTe=
mplateStoragePool: " + templatePoolRefId);
        }

If two threads are trying to stage the same template thread one gets the lo=
ck, thread two will wait.  If thread one fails to stage the template it wil=
l delete the templatePoolRef from the database.  Thread two will now get th=
e lock in op_lock, but the internal findById will not find a templatePoolRe=
f because it has been deleted and return null from acquireInLockTable().  T=
echnically thread two has the lock, but the ref templatePoolRef wasn't foun=
d.  The subsequent line "templatePoolRef =3D _tmpltPoolDao.findByPoolTempla=
te(...)" will return null, because it doesn't exist and then on the next li=
ne templatePoolRef.getState() will throw a NPE.

Darren

--_000_CE9843A44D6CEminchencitrixcom_--