ShoppingCart has very bad locking

[Adam Heath-2]

Search for synchronized in ShoppingCart.  You'll fine a couple of
spots that lock on cartLines, a few that lock in the class instance
itself.  But then no other hits.

However, tons of internal variables all need to be kept
self-consistent with each other, so locking against just one or the
other of those variables won't work.

Additionally, there are accessor methods that return direct access to
internal variables, without any locking on the return
value(ShoppingCart.iterator() is a good example of this).  This
problem also applies to the internal helper classes.

In addition, any external wrapper classes, like ProductPromoWorker,
which have to mutate the cart, do no locking whatsoever.

This can cause problems when multiple threads are trying to mutate the
cart.  Which could occur if someone clicks 'addtocart' multiple
times(like, people don't realize you only need to click it once, or
just decide to click it 5 times really fast in a row).

This is causing us much grief.  We have the 404 pages in our sites
looking pretty.  As part of this prettiness, they try to look just
like the rest of the site.  And, in the header, we have a cart status
area.  Displaying the status has to walk the items in the cart
internally, but if this is occuring at the same time you're trying to
add things to the cart, it falls over.

I don't have a solution to this.  The entire ShoppingCart subsystem
bad in this regard.

[Adrian Crum-2]

--- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:

> Search for synchronized in
> ShoppingCart.  You'll fine a couple of
> spots that lock on cartLines, a few that lock in the class
> instance
> itself.  But then no other hits.
>
> However, tons of internal variables all need to be kept
> self-consistent with each other, so locking against just
> one or the
> other of those variables won't work.

These same problems exist in a number of classes - not just the shopping cart (take a look at SequenceUtil for some really scary code). That's why I suggested a synchronization best practices page that the community can follow.

-Adrian

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:
>> Search for synchronized in
>> ShoppingCart.  You'll fine a couple of
>> spots that lock on cartLines, a few that lock in the class
>> instance
>> itself.  But then no other hits.
>>
>> However, tons of internal variables all need to be kept
>> self-consistent with each other, so locking against just
>> one or the
>> other of those variables won't work.
>
> These same problems exist in a number of classes - not just the shopping cart (take a look at SequenceUtil for some really scary code). That's why I suggested a synchronization best practices page that the community can follow.

I have looked at SequenceUtil, and have a version lying around that is
non-blocking.  I need to pull that out of the attic, and put it in my
staging tree, and run with it for a while.

[Adrian Crum-2]

--- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:

> Adrian Crum wrote:
> > --- On Fri, 3/12/10, Adam Heath <[hidden email]>
> wrote:
> >> Search for synchronized in
> >> ShoppingCart.  You'll fine a couple of
> >> spots that lock on cartLines, a few that lock in
> the class
> >> instance
> >> itself.  But then no other hits.
> >>
> >> However, tons of internal variables all need to be
> kept
> >> self-consistent with each other, so locking
> against just
> >> one or the
> >> other of those variables won't work.
> >
> > These same problems exist in a number of classes - not
> just the shopping cart (take a look at SequenceUtil for some
> really scary code). That's why I suggested a synchronization
> best practices page that the community can follow.
>
> I have looked at SequenceUtil, and have a version lying
> around that is
> non-blocking.  I need to pull that out of the attic,
> and put it in my
> staging tree, and run with it for a while.

It would be cool if you could send it to me. I can get SequenceUtil to break fairly easily with my multi-threaded data loading modification - so I think it would be a good test for your code.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:
>> Adrian Crum wrote:
>>> --- On Fri, 3/12/10, Adam Heath <[hidden email]>
>> wrote:
>>>> Search for synchronized in
>>>> ShoppingCart.  You'll fine a couple of
>>>> spots that lock on cartLines, a few that lock in
>> the class
>>>> instance
>>>> itself.  But then no other hits.
>>>>
>>>> However, tons of internal variables all need to be
>> kept
>>>> self-consistent with each other, so locking
>> against just
>>>> one or the
>>>> other of those variables won't work.
>>> These same problems exist in a number of classes - not
>> just the shopping cart (take a look at SequenceUtil for some
>> really scary code). That's why I suggested a synchronization
>> best practices page that the community can follow.
>>
>> I have looked at SequenceUtil, and have a version lying
>> around that is
>> non-blocking.  I need to pull that out of the attic,
>> and put it in my
>> staging tree, and run with it for a while.
>
> It would be cool if you could send it to me. I can get SequenceUtil to break fairly easily with my multi-threaded data loading modification - so I think it would be a good test for your code.

I'm having a hard time finding it.

However, I'll take the time to ask a question.  Why does
SequenceUtil.SequenceBank.fillBank do direct sql calls, and not use
the delegator to update values?

I'm thinking about adding test cases for SequenceUtil, but I'd like to
be able to test it in stand-alone mode.  Would being able to do small
ofbiz startups, ie, with all components upto and including the current
one, just to run the current components test cases, be a useful feature?

[Adrian Crum-2]

--- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:

> Adrian Crum wrote:
> > --- On Fri, 3/12/10, Adam Heath <[hidden email]>
> wrote:
> >> Adrian Crum wrote:
> >>> --- On Fri, 3/12/10, Adam Heath <[hidden email]>
> >> wrote:
> >>>> Search for synchronized in
> >>>> ShoppingCart.  You'll fine a couple
> of
> >>>> spots that lock on cartLines, a few that
> lock in
> >> the class
> >>>> instance
> >>>> itself.  But then no other hits.
> >>>>
> >>>> However, tons of internal variables all
> need to be
> >> kept
> >>>> self-consistent with each other, so
> locking
> >> against just
> >>>> one or the
> >>>> other of those variables won't work.
> >>> These same problems exist in a number of
> classes - not
> >> just the shopping cart (take a look at
> SequenceUtil for some
> >> really scary code). That's why I suggested a
> synchronization
> >> best practices page that the community can
> follow.
> >>
> >> I have looked at SequenceUtil, and have a version
> lying
> >> around that is
> >> non-blocking.  I need to pull that out of the
> attic,
> >> and put it in my
> >> staging tree, and run with it for a while.
> >
> > It would be cool if you could send it to me. I can get
> SequenceUtil to break fairly easily with my multi-threaded
> data loading modification - so I think it would be a good
> test for your code.
>
> I'm having a hard time finding it.
>
> However, I'll take the time to ask a question.  Why
> does
> SequenceUtil.SequenceBank.fillBank do direct sql calls, and
> not use
> the delegator to update values?
>
> I'm thinking about adding test cases for SequenceUtil, but
> I'd like to
> be able to test it in stand-alone mode.  Would being
> able to do small
> ofbiz startups, ie, with all components upto and including
> the current
> one, just to run the current components test cases, be a
> useful feature?

Of course it would be useful! Is that possible?

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:
>> Adrian Crum wrote:
>>> --- On Fri, 3/12/10, Adam Heath <[hidden email]>
>> wrote:
>>>> Search for synchronized in
>>>> ShoppingCart.  You'll fine a couple of
>>>> spots that lock on cartLines, a few that lock in
>> the class
>>>> instance
>>>> itself.  But then no other hits.
>>>>
>>>> However, tons of internal variables all need to be
>> kept
>>>> self-consistent with each other, so locking
>> against just
>>>> one or the
>>>> other of those variables won't work.
>>> These same problems exist in a number of classes - not
>> just the shopping cart (take a look at SequenceUtil for some
>> really scary code). That's why I suggested a synchronization
>> best practices page that the community can follow.
>>
>> I have looked at SequenceUtil, and have a version lying
>> around that is
>> non-blocking.  I need to pull that out of the attic,
>> and put it in my
>> staging tree, and run with it for a while.
>
> It would be cool if you could send it to me. I can get SequenceUtil to break fairly easily with my multi-threaded data loading modification - so I think it would be a good test for your code.

Was it an exception?  What kind of error was it?

[Adrian Crum-2]

--- On Fri, 3/12/10, Adam Heath <[hidden email]> wrote:

> Adrian Crum wrote:
> > --- On Fri, 3/12/10, Adam Heath <[hidden email]>
> wrote:
> >> Adrian Crum wrote:
> >>> --- On Fri, 3/12/10, Adam Heath <[hidden email]>
> >> wrote:
> >>>> Search for synchronized in
> >>>> ShoppingCart.  You'll fine a couple
> of
> >>>> spots that lock on cartLines, a few that
> lock in
> >> the class
> >>>> instance
> >>>> itself.  But then no other hits.
> >>>>
> >>>> However, tons of internal variables all
> need to be
> >> kept
> >>>> self-consistent with each other, so
> locking
> >> against just
> >>>> one or the
> >>>> other of those variables won't work.
> >>> These same problems exist in a number of
> classes - not
> >> just the shopping cart (take a look at
> SequenceUtil for some
> >> really scary code). That's why I suggested a
> synchronization
> >> best practices page that the community can
> follow.
> >>
> >> I have looked at SequenceUtil, and have a version
> lying
> >> around that is
> >> non-blocking.  I need to pull that out of the
> attic,
> >> and put it in my
> >> staging tree, and run with it for a while.
> >
> > It would be cool if you could send it to me. I can get
> SequenceUtil to break fairly easily with my multi-threaded
> data loading modification - so I think it would be a good
> test for your code.
>
> Was it an exception?  What kind of error was it?

Consistent random foreign key violations. Two records would get the same sequence number. Random in that different entities would be affected each time, and consistent in that it happens nearly every time.

Try running five or more threads against it on a multi-processor machine.

[Adrian Crum-2]

--- On Fri, 3/12/10, Adrian Crum <[hidden email]> wrote:
> Consistent random foreign key violations. Two records would
> get the same sequence number. Random in that different
> entities would be affected each time, and consistent in that
> it happens nearly every time.
>
> Try running five or more threads against it on a
> multi-processor machine.

Oops, maybe those were primary key violations. Bottom line is, an exception was thrown because two records got the same sequence ID.



     

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/12/10, Adrian Crum <[hidden email]> wrote:
>> Consistent random foreign key violations. Two records would
>> get the same sequence number. Random in that different
>> entities would be affected each time, and consistent in that
>> it happens nearly every time.
>>
>> Try running five or more threads against it on a
>> multi-processor machine.
>
> Oops, maybe those were primary key violations. Bottom line is, an exception was thrown because two records got the same sequence ID.

Well, a quick look, shows the refresh call has no locking while
updating curSeqId, which is also used by getNextSeqId and fillBank.

fillBank doesn't need to be synchronized, only the public methods,
which would still honor the contract.

Everything called by the constructor doesn't need to be synchronized
either, as object construction synchronization is based on the calling
code.

I hate DCL, it's stupid.

I'm currently doing a few little code cleanups in this file, moving
some things around, changing visibility, removing uneeded references.
 The end result will be no functional changes, the algo won't change
either, but at least should make the class follow ofbiz design
patterns better.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Fri, 3/12/10, Adrian Crum <[hidden email]> wrote:
>> Consistent random foreign key violations. Two records would
>> get the same sequence number. Random in that different
>> entities would be affected each time, and consistent in that
>> it happens nearly every time.
>>
>> Try running five or more threads against it on a
>> multi-processor machine.
>
> Oops, maybe those were primary key violations. Bottom line is, an exception was thrown because two records got the same sequence ID.

I've created a test case that runs 100 threads, all trying to get 1000
sequence values.  I then verify that I get 100,0000 different values,
and everything passes.

Could it be that your multi-threaded wrapper ends up using different
random delegators?  This would say that the locking issues lie in the
retry loop, which isn't being run by my multi-threaded code, since the
bank in each thread is the same, and synchronized handles that
concurrency.

[Adrian Crum-2]

--- On Sat, 3/13/10, Adam Heath <[hidden email]> wrote:

> Adrian Crum wrote:
> > --- On Fri, 3/12/10, Adrian Crum <[hidden email]>
> wrote:
> >> Consistent random foreign key violations. Two
> records would
> >> get the same sequence number. Random in that
> different
> >> entities would be affected each time, and
> consistent in that
> >> it happens nearly every time.
> >>
> >> Try running five or more threads against it on a
> >> multi-processor machine.
> >
> > Oops, maybe those were primary key violations. Bottom
> line is, an exception was thrown because two records got the
> same sequence ID.
>
> I've created a test case that runs 100 threads, all trying
> to get 1000
> sequence values.  I then verify that I get 100,0000
> different values,
> and everything passes.
>
> Could it be that your multi-threaded wrapper ends up using
> different
> random delegators?  This would say that the locking
> issues lie in the
> retry loop, which isn't being run by my multi-threaded
> code, since the
> bank in each thread is the same, and synchronized handles
> that
> concurrency.

I modified the demo data loading code to make it multi-threaded, so I believe only one delegator is being used. I have the patch at work, so I can't take a look at it until Monday.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Sat, 3/13/10, Adam Heath <[hidden email]> wrote:
>> Adrian Crum wrote:
>>> --- On Fri, 3/12/10, Adrian Crum <[hidden email]>
>> wrote:
>>>> Consistent random foreign key violations. Two
>> records would
>>>> get the same sequence number. Random in that
>> different
>>>> entities would be affected each time, and
>> consistent in that
>>>> it happens nearly every time.
>>>>
>>>> Try running five or more threads against it on a
>>>> multi-processor machine.
>>> Oops, maybe those were primary key violations. Bottom
>> line is, an exception was thrown because two records got the
>> same sequence ID.
>>
>> I've created a test case that runs 100 threads, all trying
>> to get 1000
>> sequence values.  I then verify that I get 100,0000
>> different values,
>> and everything passes.
>>
>> Could it be that your multi-threaded wrapper ends up using
>> different
>> random delegators?  This would say that the locking
>> issues lie in the
>> retry loop, which isn't being run by my multi-threaded
>> code, since the
>> bank in each thread is the same, and synchronized handles
>> that
>> concurrency.
>
> I modified the demo data loading code to make it multi-threaded, so I believe only one delegator is being used. I have the patch at work, so I can't take a look at it until Monday.

Heh.  I run screen, so when I go home, I can just connect back to
work, and have everything immediately available.

All remote machines I connect to I immediately install screen.  So,
when I have to continue client work from home, I can also resume
wherever I was last at.

My laptop at home has been logged in to work since Feb. 21, without
disconnecting.

Could it be the sub sequence logic that is broken?