Customer received wrong transaction number

Report · Mar 16, 2011

Hi all,

I encountered a strange problem with our shopping cart today. We had two transactions that occured around the same time. Let's say, customer A was assigned transaction number 100 and customer B transaction number 101. Both transactions were saved into the database correctly. However, customer B received an email confirmation stating that his transaction number is 100. The details of his transaction in the email are correct, only the transaction number is mixed up.

Here's roughly the code:

// get transaction number
transNum = salesCFC.createTransactionNumber()
// save shopping cart into db
salesCFC.saveTransaction( transNum, session.shoppingBagObj)
// email customer
<cfmail from="" to="" subject="Transaction Number #transNum#">
// shopping bag printout
</cfmail>

transNum is a local-scoped variable.

I have been scratching my head to understand how this happened. We only discovered this issue because customer B wanted to cancel his transaction and he gave the transaction number belonging to customer A. We checked our sent mail folder and sure enough both customers received confirmation emails with the same transaction number.

Is it possible that customer B's transNum variable got corrupted and crossed with customer A's transNum?

Report · Mar 16, 2011

My guess would be that you have a concurrency problem in salesCFC.

Dave Watts, CTO, Fig Leaf Software

http://www.figleaf.com/

http://training.figleaf.com/

Dave Watts, Eidolon LLC

Report · Mar 16, 2011

Do you mind elaborating what you meant by "concurrency problem"? The two transactions were recorded in the database with the correct transaction numbers, so I know that salesCFC.createTransactionNumber() and salesCFC.saveTransaction() are doing their jobs.

My guess would be that you have a concurrency problem in salesCFC.

Report · Mar 16, 2011

Dave's comment reminded me that I do enclose both createTransactionNumber() and saveTransaction() functions inside <cftransaction>.

So the code is (sorry I mixed in cfscript formats for readability)

// get transaction number

transNum = salesCFC.createTransactionNumber()

</cftransaction>

// save shopping cart into db

salesCFC.saveTransaction( transNum, session.shoppingBagObj)

</cftransaction>

// email customer
<cfmail from="" to="" subject="Transaction Number #transNum#">

// shopping bag printout

</cfmail>

Report · Mar 17, 2011

You are aware that any processes that rely on the transaction need to be within the *same* cftransaction tag?

Obviously I can't see what's going on inside your cfcs, but simply wrapping each query in its own transaction achieves nothing.

Report · Mar 17, 2011

I would also look into possible variable scoping issues. Do you use something like http://varscoper.riaforge.org/ to check all your code?

If there was one thing I don't like about CF, it would be the fact that variables are not local to functions by default. I can't think of any other language I have used that does this.

You say that transNum is a locally scoped variable, but you don't illustrate that in your example code. For example in CF9 prefixing all local varaibles with "local.", when accessing and assigning. How are you doing this currently?

Cheers

Report · Mar 17, 2011

Here's roughly the code:

Can you post the actual code. Including the code for createTransactionNumber(). Snip out anything that's not relevant (so code that does not use or influence the transaction number in any way... and excise any mark-up ;-), but include any branching and looping logic, and the relationship between any different files involved, incuding how each is called from other files.

It sounds like the transaction number is correct when you call saveTransaction(), but it gets overwritten between there and the CFMAIL call. To state what is probably obvious.

Perhaps at some point you are inadvertantly putting the transaction number in some shared-scope variable which is being accessed by reference rather than value, eg one can have two requests doing this:

variables.myStruct = application.myStruct;

Which might look like variables.myStruct is local, but all it is is a reference to the shared scope variable. So if one then does this:

variables.myStruct.someVariable = "some request-specific value";

Then all requests accessing variables.myStruct.someVariable will be accessing the same variable (the one in the application scope), and that variable will have a request-specific value. Bad. This will be the case for any complex objects in CF, except for arrays. I only mention this because we've quite often been caught out by this.

--

Adam

Report · Mar 17, 2011

Hi,

Thank you all for your comments. Below is the code on createTransactionNumber. Basically, the function queries the database for the most recent transaction number, and return the next number. The whole function is enclosed within <cftransaction> to ensure that both queries are run together.

    <cffunction name="createTransactionNumber" returntype="numeric">

        <cfquery name="getCurrentTransactionNum">
            select CurrentValue
            from Setting
            where VariableName = 'TransactionNum';
        </cfquery>
        <cfquery name="updateNextTransactionNumber">
            update Setting
            set CurrentValue = CurrentValue + 1
            where VariableName = 'TransactionNum';
        </cfquery>

        <cfreturn getCurrentTransactionNum.currentValue + 1 />

    </cffunction>

I am going through the codes carefully to see if that there is any problem with variables scoping and the variable referencing as suggested by Chiwi8888 and Adam. I'll report back if I find anything.

Thanks again.

Report · Mar 17, 2011

From the look of that, there's no need for you to go and get the ID upfront. Can you not just scrap the first function, run the inserts in a transaction and select back the @@IDENTITY or whatever it's called in SQL Server? You can then just pass that to your email function.

I'm pretty sure all database engines have sequencing capability, no point reinventing the wheel.

Report · Mar 17, 2011

Oh, the reason we do it our way is that we need the transaction number to send for credit card authorization. Only if the authorization is succesful do we insert the transaction into database. But I take the point about cftransaction not locking database from other threads. What is the proper way of locking it? (Not quite understand 'Select for Update' terminology). I am on SQL Server 2005. Thanks.

From the look of that, there's no need for you to go and get the ID upfront. Can you not just scrap the first function, run the inserts in a transaction and select back the @@IDENTITY or whatever it's called in SQL Server? You can then just pass that to your email function.

Report · Mar 18, 2011

Hmm, well it seems that SELECT FOR UPDATE is not available directly in SQL Server, so you'll have to bodge your own.

Inside a transaction, a row is locked when you update it. Other queries trying to access the same row will be locked until your transaction completes. Therefore I think this would work:

-- First query will lock the row

UPDATE mytable

SET myval = myval+1

WHERE mykey = 'WHATEVER'

</cfquery>

-- It's then safe to select back as we know no-one could have updated it in the meantime

SELECT myval

FROM mytable

WHERE mykey = 'WHATEVER'

</cfquery>

</cftransaction>

I don't tend to use SQL Server that much, so I'm sure someone will correct me if I'm wrong.

Incidentally (and this is more about business practise than anything else) - we do a similar thing but *always* insert into the transactions table before trying a credit card. It gets set with a status of PENDING (or similar), we then send it to the card company. On return from that, we update the row to be SUCCESS or FAILED. That way you have a full log of all transactions failed or successful (don't you want to know if someone's trying hundreds of credit cards one after the other?) and it has the side effect of getting around your issue here.

Report · Mar 18, 2011

Isn't the SQL Server equivalent "UPDLOCK" (http://msdn.microsoft.com/en-us/library/ms187373.aspx)? I'm unfamiliar with either construct, but Google seems to think that they are equivalents. And - superficial - reading of the docs for each suggests they're somewhat analogous..?

--

Adam

Report · Mar 18, 2011

Adam Cameron.

7 posts since
May 7, 2010

Interesting. I thought I'd been more of a team player than that.

Maybe they weeded out the rude / cheeky / off-topic ones? Although I can't see how that would still leave seven... 😉

--

Adam

Report · Mar 18, 2011

Interesting. I thought I'd been more of a team player than that.

No, it's weirder than that - I noticed it yesterday.

You'll find your old user on this thread. No idea what you're been up to there, I just naturally assumed you'd been drunk forumming again.

Report · Mar 18, 2011

Yeah, I noticed a coupla daze ago that my name seem to have acquired some random letters after it (not my doing...). I did not notice that I wasn't actually posting under that account any more though.

I tried to login again, but get the "new" (or "mostly disused" account). I will post from home this evening (not going to the pub tonight, sorry, so it won't be drunken) to see which account I am logged in as there.

As far as I know, I only ever had the one account though...?

Anyway, whatever.

--

Adam

Report · Mar 18, 2011

Seems Jochem has fixed it...

--

Adam

Report · Mar 18, 2011

I believe both operations would need an exclusive lock to avoid a variation of the original problem (read/write/race). Something along the lines of a serializable transaction or possibly an UPDLOCK as Adam suggested. But frankly the need to manually increment a key numeric value suggests a flaw in the table design to me.

Report · Mar 17, 2011

The whole function is enclosed
within <cftransaction> to ensure that both queries are
run together.

With the default settings, that does not block another thread from getting the same value if run at about the same time. As Owain suggested, you are better off letting database handle it. Such as using an identity column in SQL Server.

Report · Mar 17, 2011

Good spot there actually, that'll be the issue.

Doing the first select does not lock the row, as you're not doing a "SELECT FOR UPDATE". Therefore if two separate people do the select at the same time they'll both read the same value, then will both set it to the same, thus giving two people the same ID.

What database engine are you using?

Report · Mar 18, 2011

It is indeed a concurrency matter, as others have mentioned. The way your code now stands, it is possible for two users to call createTransactionNumber() at the same time, thereby obtaining the same transactionNumber.You have to prevent this from happening.

TransactionNumber is an application-wide variable. The most obvious way to prevent concurrent access is therefore to use an application lock . For example,

// get transaction number
<cflock scope = "Application" timeout = "20" type = "Exclusive">
    <cftransaction>
    <cfset transNum = salesCFC.createTransactionNumber()>
    </cftransaction>

</cflock>