postponed Initialization in multithreaded enviroment.

Discussion in 'ASP .Net' started by George, Oct 27, 2008.

  1. George

    George Guest

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

    public class clsStore
    {
    volatile List<clsPictureGroup> _lstPictureGroups = null;

    public List<clsPictureGroup> PictureGroups
    {
    get
    {
    if (_lstPictureGroups == null)
    {
    lock (this)
    {
    if (_lstPictureGroups == null)
    _lstPictureGroups = LoadPictureGroup();
    }
    }
    return _lstPictureGroups;
    }
    }
    }

    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.
     
    George, Oct 27, 2008
    #1
    1. Advertising

  2. George

    bruce barker Guest

    its not really threadsafe. you have a race condition on (_lstPictureGroups ==
    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<clsPictureGroup> _lstPictureGroups = null;
    >
    > public List<clsPictureGroup> PictureGroups
    > {
    > get
    > {
    > if (_lstPictureGroups == null)
    > {
    > lock (this)
    > {
    > if (_lstPictureGroups == null)
    > _lstPictureGroups = LoadPictureGroup();
    > }
    > }
    > return _lstPictureGroups;
    > }
    > }
    > }
    >
    > 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.
    >
    >
     
    bruce barker, Oct 27, 2008
    #2
    1. Advertising

  3. "George" <> wrote in message
    news:...
    > I am trying to implement postponed initialization (object is not
    > initialize till requested).
    >
    > public class clsStore
    > {
    > volatile List<clsPictureGroup> _lstPictureGroups = null;
    >
    > public List<clsPictureGroup> PictureGroups
    > {
    > get
    > {
    > if (_lstPictureGroups == null)
    > {
    > lock (this)
    > {
    > if (_lstPictureGroups == null)
    > _lstPictureGroups = LoadPictureGroup();
    > }
    > }
    > return _lstPictureGroups;
    > }
    > }
    > }
    >
    > 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
     
    John Saunders, Oct 27, 2008
    #3
  4. "bruce barker" <> wrote in message
    news:...
    > its not really threadsafe. you have a race condition on (_lstPictureGroups
    > ==
    > 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, Oct 27, 2008
    #4
  5. George

    George Guest

    See inline...


    "John Saunders" <> wrote in message
    news:...
    > "George" <> wrote in message
    > news:...
    >> I am trying to implement postponed initialization (object is not
    >> initialize till requested).
    >>
    >> public class clsStore
    >> {
    >> volatile List<clsPictureGroup> _lstPictureGroups = null;
    >>
    >> public List<clsPictureGroup> PictureGroups
    >> {
    >> get
    >> {
    >> if (_lstPictureGroups == null)
    >> {
    >> lock (this)
    >> {
    >> if (_lstPictureGroups == null)
    >> _lstPictureGroups = LoadPictureGroup();
    >> }
    >> }
    >> return _lstPictureGroups;
    >> }
    >> }
    >> }
    >>
    >> 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 (_lstPictureGroups == null)
    {
    lock (this)
    {
    2: if (_lstPictureGroups == null)
    _lstPictureGroups = LoadPictureGroup();


    > 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, Oct 27, 2008
    #5
  6. George

    George Guest

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

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

    What do you guys think? Valid concern?

    Thanks
    George.


    "John Saunders" <> wrote in message
    news:...
    > "bruce barker" <> wrote in message
    > news:...
    >> its not really threadsafe. you have a race condition on
    >> (_lstPictureGroups ==
    >> 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, Oct 27, 2008
    #6
  7. "George" <> wrote in message
    news:...
    > I think I am ok with that line
    > if (_lstPictureGroups == null)
    >
    > My only concern is if it allows dirty read.... Meaning that if
    > _lstPictureGroups has not been fully set and basically there is a point in
    > time when _lstPictureGroups 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, Oct 27, 2008
    #7
  8. George

    George Guest

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


    George.



    "John Saunders" <> wrote in message
    news:...
    > "George" <> wrote in message
    > news:...
    >> I think I am ok with that line
    >> if (_lstPictureGroups == null)
    >>
    >> My only concern is if it allows dirty read.... Meaning that if
    >> _lstPictureGroups has not been fully set and basically there is a point
    >> in time when _lstPictureGroups 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, Oct 27, 2008
    #8
  9. "George" <> wrote in message
    news:#...
    > It's set under lock.
    > But read happens without any regard of "lock"
    > if (_lstPictureGroups == 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
     
    John Saunders, Oct 27, 2008
    #9
  10. George

    George Guest

    >>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" <> wrote in message
    news:%...
    > "George" <> wrote in message
    > news:#...
    >> It's set under lock.
    >> But read happens without any regard of "lock"
    >> if (_lstPictureGroups == 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, Oct 27, 2008
    #10
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. vegetax
    Replies:
    5
    Views:
    403
    vegetax
    May 3, 2005
  2. vegetax
    Replies:
    1
    Views:
    393
    Scott David Daniels
    May 3, 2005
  3. Ronen Yacov
    Replies:
    7
    Views:
    332
  4. Ronen Yacov
    Replies:
    3
    Views:
    354
  5. Matthew Moss

    Ruby Quiz postponed one week

    Matthew Moss, Apr 25, 2008, in forum: Ruby
    Replies:
    0
    Views:
    86
    Matthew Moss
    Apr 25, 2008
Loading...

Share This Page