Ray was a junior developer at a large, nameless bank. One of the applications his group maintained was one that settled certain transactions. Ray was tasked with seeing if he could make the application run any faster. He discovered that the major bottleneck was processing on a network server, and decided that if he made his application multithreaded, it could settle multiple transactions at once, and it would be faster. He created a new thread object and wrote the following Execute method:
When Steven, a senior developer looked at the code, he pulled Ray aside and gently explained something very important that Ray had missed. What did Steven tell Ray?
7 Comments
The line:
Trans := TAccountTransaction.Create;
should come before the first Try, not after it.
He needs to check if GetNextPendingTransaction finds a transaction to “Settle”. It could fail if there are none left.
He needs to make sure that GetNextPendingTransaction places some kind of lock on the transaction so that multiple threads does not try to “Settle” the same transaction.
If the transaction for some reason fails he needs to make sure that it is removed from the pending transaction list so that a transaction that will fail does not stop all other transactions in the queue.
He needs to check Terminated as part of the while condition. To allow the thread to be stopped when there are still pending transactions.
while (not Terminated) and TransactionsPending do
Format, in its non-overloaded form, is not thread safe.
If Steven were going to tell Ray just one thing, I think it would that you should never make major architectural changes to a program (such as making it multithreaded) without a plan — you don’t just sit down and start writing code.
Making a plan might have addressed some of the other problems, such as Ray’s misunderstandings of exceptions and thread-safe queues. It should also have revealed that LogError probably isn’t thread-safe (although this particular call to Format _is_), and that there probably needs to be a pool of database connections instead of the single connection the program probably has now.
In addition to the tips mentioned, it is generally bad form to trap and log exceptions of type Exception. If that were EOutOfMemory or some other severe/terminal exception, then you wouldn’t want to continue running.
There are a number of other architectural assumptions that we are going to assume Ray was up on, and we don’t really have enough information to assume they don’t work multi threaded.
I’d put the outermost while loop inside the try..except block.
This avoids creating the transaction object repeatedly and let the loop breaks in case of exception.
Post a Comment