postponed Initialization in multithreaded enviroment.

[George]

I am trying to implement postponed initialization (object is not initialize
till requested).

public class clsStore
{
volatile List<clsPicture Group_lstPictur eGroups = null;

public List<clsPicture GroupPictureGro ups
{
get
{
if (_lstPictureGro ups == null)
{
lock (this)
{
if (_lstPictureGro ups == null)
_lstPictureGrou ps = LoadPictureGrou p();
}
}
return _lstPictureGrou ps;
}
}
}

Obviously this code suppose to run in multithreaded enviroment in ASP.NET.
Anyone sees any problem in this code?
As far as i can see there is no 'race' condition here and code is safe.


Thanks
George.

[John Saunders]

Re: postponed Initialization in multithreaded enviroment.

"George" <noemail@comcas t.netwrote in message
news:u7uFURFOJH A.1668@TK2MSFTN GP06.phx.gbl...

I am trying to implement postponed initialization (object is not
initialize till requested).
>
public class clsStore
{
volatile List<clsPicture Group_lstPictur eGroups = null;
>
public List<clsPicture GroupPictureGro ups
{
get
{
if (_lstPictureGro ups == null)
{
lock (this)
{
if (_lstPictureGro ups == null)
_lstPictureGrou ps = LoadPictureGrou p();
}
}
return _lstPictureGrou ps;
}
}
}
>
Obviously this code suppose to run in multithreaded enviroment in ASP.NET.
Anyone sees any problem in this code?
As far as i can see there is no 'race' condition here and code is safe.

There are a few problems.

1) Best practice is to create a separate member to act as the lock variable:
private object _locker = new object()
2) The member need not be volatile - you are only going to change it while
it is locked, right?
3) You do not need to initialize it to null, as that is the default value.
This is not C or C++ - there is a specific default value for every type, so
there is no need to initialize something just to make sure you're not
accessing uninitialized data. There _is_ no uninitialized data.

The fourth is just a matter of style. Classes are not an unusual construct
in C#. In fact, they are the norm. It is really very redundant to prefix
every class with "cls". Similarly, do you really want to prefix all of your
lists with "lst"? You will learn more from hovering the mouse over the
member in the editor than by seeing "lst", which could mean any sort of list
(unless you restrict "lst" to be only List<T>).

--
John Saunders | MVP - Connected System Developer

[=?Utf-8?B?YnJ1Y2UgYmFya2Vy?=]

RE: postponed Initialization in multithreaded enviroment.

its not really threadsafe. you have a race condition on (_lstPictureGro ups ==
null), as this is not an atomic operation (not threadsafe), nor is the
assignment. the hole is small and you would probably get away with it.

-- bruce (sqlwork.com)


"George" wrote:

I am trying to implement postponed initialization (object is not initialize
till requested).
>
public class clsStore
{
volatile List<clsPicture Group_lstPictur eGroups = null;
>
public List<clsPicture GroupPictureGro ups
{
get
{
if (_lstPictureGro ups == null)
{
lock (this)
{
if (_lstPictureGro ups == null)
_lstPictureGrou ps = LoadPictureGrou p();
}
}
return _lstPictureGrou ps;
}
}
}
>
Obviously this code suppose to run in multithreaded enviroment in ASP.NET.
Anyone sees any problem in this code?
As far as i can see there is no 'race' condition here and code is safe.
>
>
Thanks
George.
>
>

[John Saunders]

Re: postponed Initialization in multithreaded enviroment.

"bruce barker" <brucebarker@di scussions.micro soft.comwrote in message
news:2D66DCF9-B9F5-44E4-B167-D4CF5FB8FC2C@mi crosoft.com...

its not really threadsafe. you have a race condition on (_lstPictureGro ups
==
null), as this is not an atomic operation (not threadsafe), nor is the
assignment. the hole is small and you would probably get away with it.

Bruce, it is thread-safe, since he checks the value both before and after
the lock.
--
John Saunders | MVP - Connected System Developer

[George]

Re: postponed Initialization in multithreaded enviroment.

See inline...


"John Saunders" <no@dont.do.tha t.comwrote in message
news:uo8y0lFOJH A.3320@TK2MSFTN GP04.phx.gbl...

"George" <noemail@comcas t.netwrote in message
news:u7uFURFOJH A.1668@TK2MSFTN GP06.phx.gbl...

>I am trying to implement postponed initialization (object is not
>initialize till requested).
>>
> public class clsStore
> {
> volatile List<clsPicture Group_lstPictur eGroups = null;
>>
> public List<clsPicture GroupPictureGro ups
> {
> get
> {
> if (_lstPictureGro ups == null)
> {
> lock (this)
> {
> if (_lstPictureGro ups == null)
> _lstPictureGrou ps = LoadPictureGrou p();
> }
> }
> return _lstPictureGrou ps;
> }
> }
> }
>>
>Obviously this code suppose to run in multithreaded enviroment in
>ASP.NET.
>Anyone sees any problem in this code?
>As far as i can see there is no 'race' condition here and code is safe.

>
There are a few problems.
>
1) Best practice is to create a separate member to act as the lock
variable: private object _locker = new object()

I know it's best but only if you are locking (in my case clsStore) for any
other reason. If not, then i do not see any difference between lock(this) or
lock(_locker).

2) The member need not be volatile - you are only going to change it while
it is locked, right?

I think it needs to be.. I am afraid the compiler will optimize line #1 and
line #2. Is not what volatile for.... to prevent that kind of optimization.

1: if (_lstPictureGro ups == null)
{
lock (this)
{
2: if (_lstPictureGro ups == null)
_lstPictureGrou ps = LoadPictureGrou p();

3) You do not need to initialize it to null, as that is the default value.
This is not C or C++ - there is a specific default value for every type,
so there is no need to initialize something just to make sure you're not
accessing uninitialized data. There _is_ no uninitialized data.

It's just my C++ experince playing here.... Sorry, I just feel bad if i do
not initalize variable specifically :)

>
The fourth is just a matter of style. Classes are not an unusual construct
in C#. In fact, they are the norm. It is really very redundant to prefix
every class with "cls". Similarly, do you really want to prefix all of
your lists with "lst"? You will learn more from hovering the mouse over
the member in the editor than by seeing "lst", which could mean any sort
of list (unless you restrict "lst" to be only List<T>).
>

Yea, I kind of used to that style... I like Hungarian notation. All my code
has lst...txt..chk. ..map
May be I am just an old school but I find it easier.


Thanks for your comments...
George.

[George]

Re: postponed Initialization in multithreaded enviroment.

I think I am ok with that line
if (_lstPictureGro ups == null)

My only concern is if it allows dirty read.... Meaning that if
_lstPictureGrou ps has not been fully set and basically there is a point in
time when _lstPictureGrou ps returns some garbage.

What do you guys think? Valid concern?

Thanks
George.


"John Saunders" <no@dont.do.tha t.comwrote in message
news:enhDLpFOJH A.1160@TK2MSFTN GP02.phx.gbl...

"bruce barker" <brucebarker@di scussions.micro soft.comwrote in message
news:2D66DCF9-B9F5-44E4-B167-D4CF5FB8FC2C@mi crosoft.com...

>its not really threadsafe. you have a race condition on
>(_lstPictureGr oups ==
>null), as this is not an atomic operation (not threadsafe), nor is the
>assignment. the hole is small and you would probably get away with it.

>
Bruce, it is thread-safe, since he checks the value both before and after
the lock.
--
John Saunders | MVP - Connected System Developer

[John Saunders]

Re: postponed Initialization in multithreaded enviroment.

"George" <noemail@comcas t.netwrote in message
news:uMk8fGGOJH A.3876@TK2MSFTN GP04.phx.gbl...

I think I am ok with that line
if (_lstPictureGro ups == null)
>
My only concern is if it allows dirty read.... Meaning that if
_lstPictureGrou ps has not been fully set and basically there is a point in
time when _lstPictureGrou ps returns some garbage.
>
What do you guys think? Valid concern?

It's only set under lock, so I don't see that it's possible.
--
John Saunders | MVP - Connected System Developer

[George]

Re: postponed Initialization in multithreaded enviroment.

It's set under lock.
But read happens without any regard of "lock"
if (_lstPictureGro ups == null) is done without lock attempt....


George.



"John Saunders" <no@dont.do.tha t.comwrote in message
news:udhfGeGOJH A.5096@TK2MSFTN GP02.phx.gbl...

"George" <noemail@comcas t.netwrote in message
news:uMk8fGGOJH A.3876@TK2MSFTN GP04.phx.gbl...

>I think I am ok with that line
>if (_lstPictureGro ups == null)
>>
>My only concern is if it allows dirty read.... Meaning that if
>_lstPictureGro ups has not been fully set and basically there is a point
>in time when _lstPictureGrou ps returns some garbage.
>>
>What do you guys think? Valid concern?

>
It's only set under lock, so I don't see that it's possible.
--
John Saunders | MVP - Connected System Developer

[John Saunders]

Re: postponed Initialization in multithreaded enviroment.

"George" <noemail@comcas t.netwrote in message
news:#Gm4FmGOJH A.4200@TK2MSFTN GP02.phx.gbl...

It's set under lock.
But read happens without any regard of "lock"
if (_lstPictureGro ups == null) is done without lock attempt....

I'm very sure that checking a reference for null is an atomic operation in
..NET.

Also, note that he properly checks the reference twice - once before, and
once after taking out the lock. It is possible for multiple threads to see
that the field is null and attempt to acquire the lock. Only one of those
threads will acquire the lock and then find that the field is still null,
and set it to a non-null value. The other threads will acquire the lock, in
turn, and find that the field has already been set to a non-null value. They
will therefore not set it to a different non-null value.

Like the old saying, "measure twice, cut once", but this one is, "check
twice, set once".

--
John Saunders | MVP - Connected System Developer

[George]

Re: postponed Initialization in multithreaded enviroment.

>>I'm very sure that checking a reference for null is an atomic operation in

>>.NET.

I am inclined to that too.
Looks like my code is clear...:)


Thanks a lot for your comments guys..
George.


"John Saunders" <no@dont.do.tha t.comwrote in message
news:%23Fvpf3HO JHA.1960@TK2MSF TNGP04.phx.gbl. ..

"George" <noemail@comcas t.netwrote in message
news:#Gm4FmGOJH A.4200@TK2MSFTN GP02.phx.gbl...

>It's set under lock.
>But read happens without any regard of "lock"
>if (_lstPictureGro ups == null) is done without lock attempt....

>
I'm very sure that checking a reference for null is an atomic operation in
.NET.
>
Also, note that he properly checks the reference twice - once before, and
once after taking out the lock. It is possible for multiple threads to see
that the field is null and attempt to acquire the lock. Only one of those
threads will acquire the lock and then find that the field is still null,
and set it to a non-null value. The other threads will acquire the lock,
in turn, and find that the field has already been set to a non-null value.
They will therefore not set it to a different non-null value.
>
Like the old saying, "measure twice, cut once", but this one is, "check
twice, set once".
>
--
John Saunders | MVP - Connected System Developer