QuoteId and OrderId sequence, possible race condition?

[James McGill-5]

In 9.04,
We are occasionally seeing an attempt to write a duplicate QuoteId and/or
OrderId.

I'm finding it strange that for example, QuoteServices.xml#getNextQuoteId
does a read, increment, and write
instead of just using the built-in sequencer.  I wonder if we have hit some
kind of race condition, or if there is
some transaction integrity problem.  But in order to keep Quotes and Orders
going, I occasionally have to go
into PartyAcctgPreference and force an increment to the sequence.

Has anyone else seen this behavior?

--
James McGill
Phoenix AZ

[James McGill-5]

 This happened again, this time with an InvoiceId.

What could possibly cause an Invoice to be written with an InvoiceId
(received via, InvoiceServices.xml#getNextInvoiceId)
without getNextInvoiceId writing the incremented value to
PartyAcctgPreference ?

In my log I can see the last time an InvoiceId was created.
getNextInvoiceId logs its context, and the method succeeds (We are using
INVSQ_ENF_SEQ etc.).  The invoice is created and everything seems fine.

Then the next and subsequent invoices fail because the PartyAcctgPreference
wasn't updated.  I can't understand from the code how that is even possible.

Could                     <store-value value-field="partyAcctgPreference"/>
possibly not be writing, but also not throwing an error?

--
James McGill
Phoenix AZ

[Adrian Crum]

That looks like a bad design. There is no synchronization, so multiple
threads can create invoices with the same number.

I would recommend creating a Jira issue.

-Adrian

On 5/19/2010 3:30 PM, James McGill wrote:

>   This happened again, this time with an InvoiceId.
>
> What could possibly cause an Invoice to be written with an InvoiceId
> (received via, InvoiceServices.xml#getNextInvoiceId)
> without getNextInvoiceId writing the incremented value to
> PartyAcctgPreference ?
>
> In my log I can see the last time an InvoiceId was created.
> getNextInvoiceId logs its context, and the method succeeds (We are using
> INVSQ_ENF_SEQ etc.).  The invoice is created and everything seems fine.
>
> Then the next and subsequent invoices fail because the PartyAcctgPreference
> wasn't updated.  I can't understand from the code how that is even possible.
>
> Could<store-value value-field="partyAcctgPreference"/>
> possibly not be writing, but also not throwing an error?
>

[James McGill-5]

I think I may be running into this:

https://issues.apache.org/jira/browse/OFBIZ-3557

or

https://issues.apache.org/jira/browse/OFBIZ-2353

I'm a bit puzzled as to how there can be concurrency issues though.  Doesn't
getNextInvoiceId run in a transaction with createInvoice or whatever?  Does
this imply that Transactions might not be protecting against concurrency in
other situations?

This is a problem that is causing us a great deal of difficulty, because it
happens almost every day.


--
James McGill
Phoenix AZ

[James McGill-5]

On Wed, May 19, 2010 at 3:42 PM, Adrian Crum <[hidden email]> wrote:

> That looks like a bad design. There is no synchronization, so multiple
> threads can create invoices with the same number
>

Invoices, Orders, and Quotes.  The Jira tickets imply that it's a problem in
load-balanced or "heavily" multi-threaded runtimes, but it's happening to us
almost every day, on a single-processor system with just a few users.

It would be merely an annoyance if the user could submit  again, but the
problem is, the system remains broken until someone goes in and manually
increments the sequence in PartyAcctgPreference.  (And that "someone" is
me).

I checked the trunk, hoping this had been updated, but it hasn't.  I'm
surprised everyone doesn't run into this.

--
James McGill
Phoenix AZ

[Adrian Crum]

Actually, those two Jira issues are for completely separate causes. The
one that applies to you is OFBIZ-3557.

-Adrian

On 5/19/2010 4:10 PM, James McGill wrote:

> On Wed, May 19, 2010 at 3:42 PM, Adrian Crum<[hidden email]>  wrote:
>
>> That looks like a bad design. There is no synchronization, so multiple
>> threads can create invoices with the same number
>>
>
> Invoices, Orders, and Quotes.  The Jira tickets imply that it's a problem in
> load-balanced or "heavily" multi-threaded runtimes, but it's happening to us
> almost every day, on a single-processor system with just a few users.
>
> It would be merely an annoyance if the user could submit  again, but the
> problem is, the system remains broken until someone goes in and manually
> increments the sequence in PartyAcctgPreference.  (And that "someone" is
> me).
>
> I checked the trunk, hoping this had been updated, but it hasn't.  I'm
> surprised everyone doesn't run into this.
>

[David E. Jones-2]

It would be nice if it were that easy, but not only could there be multiple threads in a single app server instance but there could be multiple app servers using this at the same time.

This sounds like a transaction isolation problem, and really database transaction isolation is the only way to properly handle this.

The place to look for this problem in a production system is what the database tx isolation is set to, and check if it is handling this right.

Without handling it through the db the best option would be to write recovery code, ie call the save invoice in its own transaction and if it returns an error (and rolls back as part of that) then increment the ID (as James is doing manually) and try again.

-David


On May 19, 2010, at 4:42 PM, Adrian Crum wrote:

> That looks like a bad design. There is no synchronization, so multiple threads can create invoices with the same number.
>
> I would recommend creating a Jira issue.
>
> -Adrian
>
> On 5/19/2010 3:30 PM, James McGill wrote:
>>  This happened again, this time with an InvoiceId.
>>
>> What could possibly cause an Invoice to be written with an InvoiceId
>> (received via, InvoiceServices.xml#getNextInvoiceId)
>> without getNextInvoiceId writing the incremented value to
>> PartyAcctgPreference ?
>>
>> In my log I can see the last time an InvoiceId was created.
>> getNextInvoiceId logs its context, and the method succeeds (We are using
>> INVSQ_ENF_SEQ etc.).  The invoice is created and everything seems fine.
>>
>> Then the next and subsequent invoices fail because the PartyAcctgPreference
>> wasn't updated.  I can't understand from the code how that is even possible.
>>
>> Could<store-value value-field="partyAcctgPreference"/>
>> possibly not be writing, but also not throwing an error?
>>

[Adrian Crum-2]

I must be missing something - I don't see how transaction isolation will solve this problem. For example:

Thread A: Begin transaction
Thread B: Begin transaction
Thread A: Get last invoice number (10000)
Thread B: Get last invoice number (10000)
Thread A: Increment invoice number (10001)
Thread B: Increment invoice number (10001)
Thread A: Store new invoice number
Thread B: Store new invoice number
Thread A: Commit transaction
Thread B: Commit transaction

That last step is going to fail no matter how you have your transaction isolation set up.

A single-server setup could solve the problem by putting the invoice number generation code inside a synchronized method. In a multi-server setup, you would have to have a test-and-retry method like you suggested.

-Adrian

--- On Wed, 5/19/10, David E Jones <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: QuoteId and OrderId sequence, possible race condition?
> To: [hidden email]
> Date: Wednesday, May 19, 2010, 5:32 PM
>
> It would be nice if it were that easy, but not only could
> there be multiple threads in a single app server instance
> but there could be multiple app servers using this at the
> same time.
>
> This sounds like a transaction isolation problem, and
> really database transaction isolation is the only way to
> properly handle this.
>
> The place to look for this problem in a production system
> is what the database tx isolation is set to, and check if it
> is handling this right.
>
> Without handling it through the db the best option would be
> to write recovery code, ie call the save invoice in its own
> transaction and if it returns an error (and rolls back as
> part of that) then increment the ID (as James is doing
> manually) and try again.
>
> -David
>
>
> On May 19, 2010, at 4:42 PM, Adrian Crum wrote:
>
> > That looks like a bad design. There is no
> synchronization, so multiple threads can create invoices
> with the same number.
> >
> > I would recommend creating a Jira issue.
> >
> > -Adrian
> >
> > On 5/19/2010 3:30 PM, James McGill wrote:
> >>  This happened again, this time with an
> InvoiceId.
> >>
> >> What could possibly cause an Invoice to be written
> with an InvoiceId
> >> (received via,
> InvoiceServices.xml#getNextInvoiceId)
> >> without getNextInvoiceId writing the incremented
> value to
> >> PartyAcctgPreference ?
> >>
> >> In my log I can see the last time an InvoiceId was
> created.
> >> getNextInvoiceId logs its context, and the method
> succeeds (We are using
> >> INVSQ_ENF_SEQ etc.).  The invoice is created
> and everything seems fine.
> >>
> >> Then the next and subsequent invoices fail because
> the PartyAcctgPreference
> >> wasn't updated.  I can't understand from the
> code how that is even possible.
> >>
> >> Could<store-value
> value-field="partyAcctgPreference"/>
> >> possibly not be writing, but also not throwing an
> error?
> >>
>
>

[David E. Jones-2]

It's all about locking and blocking...

The scenario you describe is probably fairly rare (ie two processes in perfect sync), and in most cases once the sequence is updated the db should lock and block thread B, but that may note be happening. For cases where two threads do manage to both get a query in to prevent the problem in the db thread A could do a select for update (causing a lock if the db cooperates) and then thread B would block until thread commits or rolls back.

-David


On May 19, 2010, at 10:38 PM, Adrian Crum wrote:

> I must be missing something - I don't see how transaction isolation will solve this problem. For example:
>
> Thread A: Begin transaction
> Thread B: Begin transaction
> Thread A: Get last invoice number (10000)
> Thread B: Get last invoice number (10000)
> Thread A: Increment invoice number (10001)
> Thread B: Increment invoice number (10001)
> Thread A: Store new invoice number
> Thread B: Store new invoice number
> Thread A: Commit transaction
> Thread B: Commit transaction
>
> That last step is going to fail no matter how you have your transaction isolation set up.
>
> A single-server setup could solve the problem by putting the invoice number generation code inside a synchronized method. In a multi-server setup, you would have to have a test-and-retry method like you suggested.
>
> -Adrian
>
> --- On Wed, 5/19/10, David E Jones <[hidden email]> wrote:
>
>> From: David E Jones <[hidden email]>
>> Subject: Re: QuoteId and OrderId sequence, possible race condition?
>> To: [hidden email]
>> Date: Wednesday, May 19, 2010, 5:32 PM
>>
>> It would be nice if it were that easy, but not only could
>> there be multiple threads in a single app server instance
>> but there could be multiple app servers using this at the
>> same time.
>>
>> This sounds like a transaction isolation problem, and
>> really database transaction isolation is the only way to
>> properly handle this.
>>
>> The place to look for this problem in a production system
>> is what the database tx isolation is set to, and check if it
>> is handling this right.
>>
>> Without handling it through the db the best option would be
>> to write recovery code, ie call the save invoice in its own
>> transaction and if it returns an error (and rolls back as
>> part of that) then increment the ID (as James is doing
>> manually) and try again.
>>
>> -David
>>
>>
>> On May 19, 2010, at 4:42 PM, Adrian Crum wrote:
>>
>>> That looks like a bad design. There is no
>> synchronization, so multiple threads can create invoices
>> with the same number.
>>>
>>> I would recommend creating a Jira issue.
>>>
>>> -Adrian
>>>
>>> On 5/19/2010 3:30 PM, James McGill wrote:
>>>>   This happened again, this time with an
>> InvoiceId.
>>>>
>>>> What could possibly cause an Invoice to be written
>> with an InvoiceId
>>>> (received via,
>> InvoiceServices.xml#getNextInvoiceId)
>>>> without getNextInvoiceId writing the incremented
>> value to
>>>> PartyAcctgPreference ?
>>>>
>>>> In my log I can see the last time an InvoiceId was
>> created.
>>>> getNextInvoiceId logs its context, and the method
>> succeeds (We are using
>>>> INVSQ_ENF_SEQ etc.).  The invoice is created
>> and everything seems fine.
>>>>
>>>> Then the next and subsequent invoices fail because
>> the PartyAcctgPreference
>>>> wasn't updated.  I can't understand from the
>> code how that is even possible.
>>>>
>>>> Could<store-value
>> value-field="partyAcctgPreference"/>
>>>> possibly not be writing, but also not throwing an
>> error?
>>>>
>>
>>
>
>
>

[Adrian Crum-2]

That's an interesting perspective. It's been my understanding that you can read/write all you want inside a transaction, but no changes are actually made to the database until you commit the transaction. It's during the commit that any locking/blocking will occur. But I might be wrong.

-Adrian

--- On Wed, 5/19/10, David E Jones <[hidden email]> wrote:

> From: David E Jones <[hidden email]>
> Subject: Re: QuoteId and OrderId sequence, possible race condition?
> To: [hidden email]
> Date: Wednesday, May 19, 2010, 9:53 PM
>
> It's all about locking and blocking...
>
> The scenario you describe is probably fairly rare (ie two
> processes in perfect sync), and in most cases once the
> sequence is updated the db should lock and block thread B,
> but that may note be happening. For cases where two threads
> do manage to both get a query in to prevent the problem in
> the db thread A could do a select for update (causing a lock
> if the db cooperates) and then thread B would block until
> thread commits or rolls back.
>
> -David
>
>
> On May 19, 2010, at 10:38 PM, Adrian Crum wrote:
>
> > I must be missing something - I don't see how
> transaction isolation will solve this problem. For example:
> >
> > Thread A: Begin transaction
> > Thread B: Begin transaction
> > Thread A: Get last invoice number (10000)
> > Thread B: Get last invoice number (10000)
> > Thread A: Increment invoice number (10001)
> > Thread B: Increment invoice number (10001)
> > Thread A: Store new invoice number
> > Thread B: Store new invoice number
> > Thread A: Commit transaction
> > Thread B: Commit transaction
> >
> > That last step is going to fail no matter how you have
> your transaction isolation set up.
> >
> > A single-server setup could solve the problem by
> putting the invoice number generation code inside a
> synchronized method. In a multi-server setup, you would have
> to have a test-and-retry method like you suggested.
> >
> > -Adrian
> >
> > --- On Wed, 5/19/10, David E Jones <[hidden email]>
> wrote:
> >
> >> From: David E Jones <[hidden email]>
> >> Subject: Re: QuoteId and OrderId sequence,
> possible race condition?
> >> To: [hidden email]
> >> Date: Wednesday, May 19, 2010, 5:32 PM
> >>
> >> It would be nice if it were that easy, but not
> only could
> >> there be multiple threads in a single app server
> instance
> >> but there could be multiple app servers using this
> at the
> >> same time.
> >>
> >> This sounds like a transaction isolation problem,
> and
> >> really database transaction isolation is the only
> way to
> >> properly handle this.
> >>
> >> The place to look for this problem in a production
> system
> >> is what the database tx isolation is set to, and
> check if it
> >> is handling this right.
> >>
> >> Without handling it through the db the best option
> would be
> >> to write recovery code, ie call the save invoice
> in its own
> >> transaction and if it returns an error (and rolls
> back as
> >> part of that) then increment the ID (as James is
> doing
> >> manually) and try again.
> >>
> >> -David
> >>
> >>
> >> On May 19, 2010, at 4:42 PM, Adrian Crum wrote:
> >>
> >>> That looks like a bad design. There is no
> >> synchronization, so multiple threads can create
> invoices
> >> with the same number.
> >>>
> >>> I would recommend creating a Jira issue.
> >>>
> >>> -Adrian
> >>>
> >>> On 5/19/2010 3:30 PM, James McGill wrote:
> >>>>   This happened again, this
> time with an
> >> InvoiceId.
> >>>>
> >>>> What could possibly cause an Invoice to be
> written
> >> with an InvoiceId
> >>>> (received via,
> >> InvoiceServices.xml#getNextInvoiceId)
> >>>> without getNextInvoiceId writing the
> incremented
> >> value to
> >>>> PartyAcctgPreference ?
> >>>>
> >>>> In my log I can see the last time an
> InvoiceId was
> >> created.
> >>>> getNextInvoiceId logs its context, and the
> method
> >> succeeds (We are using
> >>>> INVSQ_ENF_SEQ etc.).  The invoice is
> created
> >> and everything seems fine.
> >>>>
> >>>> Then the next and subsequent invoices fail
> because
> >> the PartyAcctgPreference
> >>>> wasn't updated.  I can't understand
> from the
> >> code how that is even possible.
> >>>>
> >>>> Could<store-value
> >> value-field="partyAcctgPreference"/>
> >>>> possibly not be writing, but also not
> throwing an
> >> error?
> >>>>
> >>
> >>
> >
> >
> >
>
>

[James McGill-5]

On Wed, May 19, 2010 at 9:53 PM, David E Jones <[hidden email]> wrote:

>
> It's all about locking and blocking...
>
> The scenario you describe is probably fairly rare


Probably, but it has hit us several times, and we don't have a lot of users
on this.  When it breaks, it stays broken until someone updates
PartyAcctgPreference.


> For cases where two threads do manage to both get a query in to prevent the
> problem in the db thread A could do a select for update (causing a lock if
> the db cooperates)


I don't think minilang provides this much control.   How do you do select
for update via the delegator?

SequenceUtil relies on JVM synchronization, so that should work for this
also.


James

[Adrian Crum]

James,

I uploaded a patch that might fix your problem:

https://issues.apache.org/jira/browse/OFBIZ-3557

-Adrian

On 5/20/2010 10:31 AM, James McGill wrote:

> On Wed, May 19, 2010 at 9:53 PM, David E Jones<[hidden email]>  wrote:
>
>>
>> It's all about locking and blocking...
>>
>> The scenario you describe is probably fairly rare
>
>
> Probably, but it has hit us several times, and we don't have a lot of users
> on this.  When it breaks, it stays broken until someone updates
> PartyAcctgPreference.
>
>
>> For cases where two threads do manage to both get a query in to prevent the
>> problem in the db thread A could do a select for update (causing a lock if
>> the db cooperates)
>
>
> I don't think minilang provides this much control.   How do you do select
> for update via the delegator?
>
> SequenceUtil relies on JVM synchronization, so that should work for this
> also.
>
>
> James
>

[James McGill-5]

On Thu, May 20, 2010 at 10:46 AM, Adrian Crum <[hidden email]> wrote:

> James,
>
> I uploaded a patch that might fix your problem:
>
>
Thanks Adrian.  Your approach adds JVM-based synchronization to minilang
simple methods.

I still wonder if this means the create services need to be synchronized and
not just the id generators.

I'll try it out and keep you posted.


James

[Adrian Crum]

I doubt there will be any need for additional synchronization.

I think I know what's going on in your system. My previous scenario
wasn't correct because I thought the ID was a part of the primary key
and there was an exception thrown while storing the new value - due to a
duplicate key violation. Those IDs are not a part of the primary key, so
that isn't the cause.

Instead, I believe the ID is updated successfully, but a thread is slow
in storing the new ID value - which ends up decrementing the ID in the
database instead of incrementing it. Then the invoice generation fails
due to a duplicate key violation. From that point on, every attempt to
increment the ID value and create a new invoice with it will fail
because the new ID value already exists in the invoice file.

-Adrian

On 5/20/2010 11:57 AM, James McGill wrote:

> On Thu, May 20, 2010 at 10:46 AM, Adrian Crum<[hidden email]>  wrote:
>
>> James,
>>
>> I uploaded a patch that might fix your problem:
>>
>>
> Thanks Adrian.  Your approach adds JVM-based synchronization to minilang
> simple methods.
>
> I still wonder if this means the create services need to be synchronized and
> not just the id generators.
>
> I'll try it out and keep you posted.
>
>
> James
>

[James McGill-5]

For completeness' sake, the XSD

Index: framework/minilang/dtd/simple-methods.xsd
===================================================================
--- framework/minilang/dtd/simple-methods.xsd   (revision 2187)
+++ framework/minilang/dtd/simple-methods.xsd   (working copy)
@@ -223,6 +223,19 @@
                 </xs:restriction>
             </xs:simpleType>
         </xs:attribute>
+        <xs:attribute name="synchronized" default="false">
+            <xs:annotation>
+                <xs:documentation>
+                                       Run the method under JVM
synchronized keyword
+                </xs:documentation>
+            </xs:annotation>
+            <xs:simpleType>
+                <xs:restriction base="xs:token">
+                    <xs:enumeration value="true"/>
+                    <xs:enumeration value="false"/>
+                </xs:restriction>
+            </xs:simpleType>
+        </xs:attribute>
         <xs:attribute type="xs:string" name="default-error-code"
default="error">
             <xs:annotation>
                 <xs:documentation>

[Adrian Crum]

Thanks! I was going to do that once I had confirmation the patch fixes
the problem.

-Adrian

On 5/20/2010 3:52 PM, James McGill wrote:

> For completeness' sake, the XSD
>
> Index: framework/minilang/dtd/simple-methods.xsd
> ===================================================================
> --- framework/minilang/dtd/simple-methods.xsd   (revision 2187)
> +++ framework/minilang/dtd/simple-methods.xsd   (working copy)
> @@ -223,6 +223,19 @@
>                   </xs:restriction>
>               </xs:simpleType>
>           </xs:attribute>
> +<xs:attribute name="synchronized" default="false">
> +<xs:annotation>
> +<xs:documentation>
> +                                       Run the method under JVM
> synchronized keyword
> +</xs:documentation>
> +</xs:annotation>
> +<xs:simpleType>
> +<xs:restriction base="xs:token">
> +<xs:enumeration value="true"/>
> +<xs:enumeration value="false"/>
> +</xs:restriction>
> +</xs:simpleType>
> +</xs:attribute>
>           <xs:attribute type="xs:string" name="default-error-code"
> default="error">
>               <xs:annotation>
>                   <xs:documentation>
>

[Matt Warnock]

Isn't this exactly the kind of thing that SQL transactions, sequences,
and foreign key constraints are designed to solve?  

If the thread is using transactions properly, the whole transaction
should either be rolled back, or completed.  Either way, the database
SHOULD be in a wholly consistent state.  But it obviously isn't.

So either we are doing something outside a transaction, that really
should be done INSIDE a transaction, or the database has a seriously
flawed implementation of transactions.
--
Matt Warnock <[hidden email]>
RidgeCrest Herbals, Inc.

On Thu, 2010-05-20 at 12:43 -0700, Adrian Crum wrote:

> I doubt there will be any need for additional synchronization.
>
> I think I know what's going on in your system. My previous scenario
> wasn't correct because I thought the ID was a part of the primary key
> and there was an exception thrown while storing the new value - due to a
> duplicate key violation. Those IDs are not a part of the primary key, so
> that isn't the cause.
>
> Instead, I believe the ID is updated successfully, but a thread is slow
> in storing the new ID value - which ends up decrementing the ID in the
> database instead of incrementing it. Then the invoice generation fails
> due to a duplicate key violation. From that point on, every attempt to
> increment the ID value and create a new invoice with it will fail
> because the new ID value already exists in the invoice file.
>
> -Adrian
>
> On 5/20/2010 11:57 AM, James McGill wrote:
> > On Thu, May 20, 2010 at 10:46 AM, Adrian Crum<[hidden email]>  wrote:
> >
> >> James,
> >>
> >> I uploaded a patch that might fix your problem:
> >>
> >>
> > Thanks Adrian.  Your approach adds JVM-based synchronization to minilang
> > simple methods.
> >
> > I still wonder if this means the create services need to be synchronized and
> > not just the id generators.
> >
> > I'll try it out and keep you posted.
> >
> >
> > James
> >

[Adrian Crum-2]

Matt,

If you follow the thread from the start and visit the Jira issue, you will see that we are discussing a very specific problem - not transactions or sequences in general.

To summarize: There is code in OFBiz that generates sequential ID numbers for orders and invoices. That code does not follow the established pattern for generating sequences, and as a result it will produce non-sequential IDs. A Jira issue has been created and we are working on the problem.

-Adrian

--- On Thu, 5/20/10, Matt Warnock <[hidden email]> wrote:

> From: Matt Warnock <[hidden email]>
> Subject: Re: QuoteId and OrderId sequence, possible race condition?
> To: [hidden email]
> Date: Thursday, May 20, 2010, 5:15 PM
> Isn't this exactly the kind of thing
> that SQL transactions, sequences,
> and foreign key constraints are designed to solve? 
>
> If the thread is using transactions properly, the whole
> transaction
> should either be rolled back, or completed.  Either
> way, the database
> SHOULD be in a wholly consistent state.  But it
> obviously isn't.
>
> So either we are doing something outside a transaction,
> that really
> should be done INSIDE a transaction, or the database has a
> seriously
> flawed implementation of transactions.
> --
> Matt Warnock <[hidden email]>
> RidgeCrest Herbals, Inc.
>
> On Thu, 2010-05-20 at 12:43 -0700, Adrian Crum wrote:
> > I doubt there will be any need for additional
> synchronization.
> >
> > I think I know what's going on in your system. My
> previous scenario
> > wasn't correct because I thought the ID was a part of
> the primary key
> > and there was an exception thrown while storing the
> new value - due to a
> > duplicate key violation. Those IDs are not a part of
> the primary key, so
> > that isn't the cause.
> >
> > Instead, I believe the ID is updated successfully, but
> a thread is slow
> > in storing the new ID value - which ends up
> decrementing the ID in the
> > database instead of incrementing it. Then the invoice
> generation fails
> > due to a duplicate key violation. From that point on,
> every attempt to
> > increment the ID value and create a new invoice with
> it will fail
> > because the new ID value already exists in the invoice
> file.
> >
> > -Adrian
> >
> > On 5/20/2010 11:57 AM, James McGill wrote:
> > > On Thu, May 20, 2010 at 10:46 AM, Adrian
> Crum<[hidden email]> 
> wrote:
> > >
> > >> James,
> > >>
> > >> I uploaded a patch that might fix your
> problem:
> > >>
> > >>
> > > Thanks Adrian.  Your approach adds JVM-based
> synchronization to minilang
> > > simple methods.
> > >
> > > I still wonder if this means the create services
> need to be synchronized and
> > > not just the id generators.
> > >
> > > I'll try it out and keep you posted.
> > >
> > >
> > > James
> > >
>
>

[Matt Warnock]

Adrian:

I had been following it (with some interest) from the start, but went
back and read it again.  Also reviewed the JIRA issue, which I had not
previously.  I have also reviewed the patch you submitted, though I am
not altogether certain what "special" invoice order sequencing means in
this context, and in any event, I am no XML or ofbiz expert (yet).  But
is seems that IF "special" sequencing is in effect, then this race
condition occurs.

Now, MOST businesses will require that every invoice number is unique,
and MANY will further require that all order/invoice/check numbers be
accounted for, even on prenumbered forms (no "gaps") for auditing
purposes.  Is that "special" to ofbiz?  

Alternatively, some businesses I have seen enforce a particular format
on the invoice/order/document number (like OOOYYYYMMDDSSS, where OOO is
the office number, followed by a date, and SSS is a sequence number
within the day), and maybe that is where the trouble lies.  In the US,
Social Security numbers are generated on a "special" pattern like this.

In either event, I remain convinced, and on reviewing the thread, David
Jones seems to agree, that this problem cannot be easily solved except
with atomic SQL transactions.  Is there some reason they are not being
used in this situation?  Or are they in use, and is the OP using a
REALLY broken JDBC? Or are they in use, but a record write has
inadvertently snuck outside the transaction?

At our company, we have two people putting orders in all morning (and we
are a very small company), and the chance of a collision in your
scenario is NOT rare, but quite high indeed, as the OP has already
pointed out, even on a single server with multiple users.  But we would
only find this out after rollout, not during single-user testing, and it
would be an apparently random system lockup, which would be frustrating
to say the least.  Kudos to the OP for spotting the real nature of the
problem-- race conditions are often hard to identify.  :-)

The issue is not that the sequence generator is "slow" to store the new
invoice number, but rather that "last ID" read, the "new ID" generator,
and the "new ID" write are not ALL encased inside a transaction that
will ensure that the database is ALWAYS internally consistent.  The
"race" occurs because there is no enclosing transaction, and the "race"
will never be completely controlled by being "fast" enough.

If there is a long lag between read and write (as where a user is
entering order line items) then you either have to 1) do the
read/increment/write at the END of the data entry process, or 2) face
the high likelihood that two entry processes will collide, and one of
them will have to roll back and then be renumbered.  

Most software I have seen is not very bright about this, and it reads
and generates the new order number first, displaying it on the input
screen as data is entered.  That is not the SQL-friendly way, and asks
for trouble.  Users like it, because they often need to write that
number down on the data entry form--  they just need to be trained to
get that order ID as the last thing they do. :(  I'm strongly hoping
ofbiz will conform to better practices here, as it has elsewhere.

Whether you use a SQL standard "sequence" or not (or even the mysql
"autoincrement" abomination), and regardless of the primary key status
of the orderID (any SQL "unique" declaration could have the same effect,
and a well designed order table should at LEAST have a unique orderID,
whether it is part of the primary key or not), SQL transactions should
resolve this issue easily and scalably.  

On the other hand, using locking or semaphores in the application code
is reinventing the SQL wheel and fraught with possible error.  As David
suggested, there may be multiple front ends talking to a single
database, so a SQL solution is best.  I am merely respectfully
suggesting that it would be better to let SQL do what it does best, and
atomic transactions are the essence of good SQL.  

Hope this helps explain where I am coming from.

--
Matt Warnock <[hidden email]>
RidgeCrest Herbals, Inc.

On Thu, 2010-05-20 at 22:26 -0700, Adrian Crum wrote:

> Matt,
>
> If you follow the thread from the start and visit the Jira issue, you will see that we are discussing a very specific problem - not transactions or sequences in general.
>
> To summarize: There is code in OFBiz that generates sequential ID numbers for orders and invoices. That code does not follow the established pattern for generating sequences, and as a result it will produce non-sequential IDs. A Jira issue has been created and we are working on the problem.
>
> -Adrian
>
> --- On Thu, 5/20/10, Matt Warnock <[hidden email]> wrote:
>
> > From: Matt Warnock <[hidden email]>
> > Subject: Re: QuoteId and OrderId sequence, possible race condition?
> > To: [hidden email]
> > Date: Thursday, May 20, 2010, 5:15 PM
> > Isn't this exactly the kind of thing
> > that SQL transactions, sequences,
> > and foreign key constraints are designed to solve?  
> >
> > If the thread is using transactions properly, the whole
> > transaction
> > should either be rolled back, or completed.  Either
> > way, the database
> > SHOULD be in a wholly consistent state.  But it
> > obviously isn't.
> >
> > So either we are doing something outside a transaction,
> > that really
> > should be done INSIDE a transaction, or the database has a
> > seriously
> > flawed implementation of transactions.
> > --
> > Matt Warnock <[hidden email]>
> > RidgeCrest Herbals, Inc.
> >
> > On Thu, 2010-05-20 at 12:43 -0700, Adrian Crum wrote:
> > > I doubt there will be any need for additional
> > synchronization.
> > >
> > > I think I know what's going on in your system. My
> > previous scenario
> > > wasn't correct because I thought the ID was a part of
> > the primary key
> > > and there was an exception thrown while storing the
> > new value - due to a
> > > duplicate key violation. Those IDs are not a part of
> > the primary key, so
> > > that isn't the cause.
> > >
> > > Instead, I believe the ID is updated successfully, but
> > a thread is slow
> > > in storing the new ID value - which ends up
> > decrementing the ID in the
> > > database instead of incrementing it. Then the invoice
> > generation fails
> > > due to a duplicate key violation. From that point on,
> > every attempt to
> > > increment the ID value and create a new invoice with
> > it will fail
> > > because the new ID value already exists in the invoice
> > file.
> > >
> > > -Adrian
> > >
> > > On 5/20/2010 11:57 AM, James McGill wrote:
> > > > On Thu, May 20, 2010 at 10:46 AM, Adrian
> > Crum<[hidden email]>
> > wrote:
> > > >
> > > >> James,
> > > >>
> > > >> I uploaded a patch that might fix your
> > problem:
> > > >>
> > > >>
> > > > Thanks Adrian.  Your approach adds JVM-based
> > synchronization to minilang
> > > > simple methods.
> > > >
> > > > I still wonder if this means the create services
> > need to be synchronized and
> > > > not just the id generators.
> > > >
> > > > I'll try it out and keep you posted.
> > > >
> > > >
> > > > James
> > > >
> >
> >
>
>
>      

[Malin Nicolas]

Hello,

On QuoteId and OrderId, I create a jira to manage id generation by
service define in customMethod. After we can add new service by business
orientation.

https://issues.apache.org/jira/browse/OFBIZ-3765

I wait a commiter review ;)

Nicolas

Matt Warnock a écrit :

> Adrian:
>
> I had been following it (with some interest) from the start, but went
> back and read it again.  Also reviewed the JIRA issue, which I had not
> previously.  I have also reviewed the patch you submitted, though I am
> not altogether certain what "special" invoice order sequencing means in
> this context, and in any event, I am no XML or ofbiz expert (yet).  But
> is seems that IF "special" sequencing is in effect, then this race
> condition occurs.
>
> Now, MOST businesses will require that every invoice number is unique,
> and MANY will further require that all order/invoice/check numbers be
> accounted for, even on prenumbered forms (no "gaps") for auditing
> purposes.  Is that "special" to ofbiz?  
>
> Alternatively, some businesses I have seen enforce a particular format
> on the invoice/order/document number (like OOOYYYYMMDDSSS, where OOO is
> the office number, followed by a date, and SSS is a sequence number
> within the day), and maybe that is where the trouble lies.  In the US,
> Social Security numbers are generated on a "special" pattern like this.
>
> In either event, I remain convinced, and on reviewing the thread, David
> Jones seems to agree, that this problem cannot be easily solved except
> with atomic SQL transactions.  Is there some reason they are not being
> used in this situation?  Or are they in use, and is the OP using a
> REALLY broken JDBC? Or are they in use, but a record write has
> inadvertently snuck outside the transaction?
>
> At our company, we have two people putting orders in all morning (and we
> are a very small company), and the chance of a collision in your
> scenario is NOT rare, but quite high indeed, as the OP has already
> pointed out, even on a single server with multiple users.  But we would
> only find this out after rollout, not during single-user testing, and it
> would be an apparently random system lockup, which would be frustrating
> to say the least.  Kudos to the OP for spotting the real nature of the
> problem-- race conditions are often hard to identify.  :-)
>
> The issue is not that the sequence generator is "slow" to store the new
> invoice number, but rather that "last ID" read, the "new ID" generator,
> and the "new ID" write are not ALL encased inside a transaction that
> will ensure that the database is ALWAYS internally consistent.  The
> "race" occurs because there is no enclosing transaction, and the "race"
> will never be completely controlled by being "fast" enough.
>
> If there is a long lag between read and write (as where a user is
> entering order line items) then you either have to 1) do the
> read/increment/write at the END of the data entry process, or 2) face
> the high likelihood that two entry processes will collide, and one of
> them will have to roll back and then be renumbered.  
>
> Most software I have seen is not very bright about this, and it reads
> and generates the new order number first, displaying it on the input
> screen as data is entered.  That is not the SQL-friendly way, and asks
> for trouble.  Users like it, because they often need to write that
> number down on the data entry form--  they just need to be trained to
> get that order ID as the last thing they do. :(  I'm strongly hoping
> ofbiz will conform to better practices here, as it has elsewhere.
>
> Whether you use a SQL standard "sequence" or not (or even the mysql
> "autoincrement" abomination), and regardless of the primary key status
> of the orderID (any SQL "unique" declaration could have the same effect,
> and a well designed order table should at LEAST have a unique orderID,
> whether it is part of the primary key or not), SQL transactions should
> resolve this issue easily and scalably.  
>
> On the other hand, using locking or semaphores in the application code
> is reinventing the SQL wheel and fraught with possible error.  As David
> suggested, there may be multiple front ends talking to a single
> database, so a SQL solution is best.  I am merely respectfully
> suggesting that it would be better to let SQL do what it does best, and
> atomic transactions are the essence of good SQL.  
>
> Hope this helps explain where I am coming from.
>
>