Ensuring Thread Safety

Question

I am in the midst of writing a C# Windows Form application that processes quotes from the market through an algorithm (strategy) to create orders to a brokerage firm. That all seems to be testing fairly well until I tried to build in the capacity to run multiple strategies simultaneously with each strategy on it's own thread. At this point everything starts running incorrectly. I believe I have some classes that are not thread safe which are driving erratic behavior. Any insight on how I can thread this in a thread safe manner is deeply appreciated!

The way the Quotes are fed into the Algorithms is as Follows: 1) Market Data Events are fired from Brokers Software to a client class in my Software called ConnectionStatus. When the market Data event is triggered, a Quote object is built from the current values of these static variables that represent Bid, ask, etc. Once the quote is built, I am endeavoring to send it into each of the Strategy Algorithms that are running. Here is the code I am using to do that:

 foreach (StrategyAssembler assembler in StrategyAssembleList.GetStrategies())
 {                  
     BackgroundWorker thread = strategyThreadPool.GetFreeThread();
     if (thread != null)
     {
        thread.DoWork += new DoWorkEventHandler(assembler.NewIncomingQuote);
        thread.RunWorkerAsync(quote);
     }   
 }

StrategyAssembler is a class that creates an instance of the Class StrategyManager which in turn creates an instance of the strategy that contains the actual algorithms. There may be 4 or 6 different instances of StrategyAssembler, each of which has been added to a Singleton instance of StrategyAssembleList which is a BindingList.

The incoming quote object is passed into the the NewIncomingQuote method of the StrategyAssembler Class. That code is as follows:

public void NewIncomingQuote(object sender, DoWorkEventArgs e)
    {
        Quote QUOTE = e.Argument as Quote;            

        lock (QuoteLocker)
        {
            manager.LiveQuote(QUOTE);

            priorQuote = QUOTE;
        }
    }

I was thinking that by using a lock before passing the quote into the manager.LiveQuote(Quote quote) method that all the objects that use the quote "downstream" of this point would be able to consume the quote in a thread safe fashion, but testing is showing otherwise. Is there a way I could put the each instance Of StrategyAssembler on its own thread that ensures that all the objects created by Strategy Assembler are thread safe and then feed the quote into StrategyAssembler? Is this path of thinking an appropriate way to deal with this situation?

Thanks in advance for any feedback or help,

Learning1

1) What does `manager.LiveQuote()` do? Is it the interface to your strategy methods? 2) Is `QuoteLocker` a `StrategyAssembler` instance member? 3) Finally, what kind of behavior are you expecting and what are you seeing? — Jeff Sternal, Mar 01 '10 at 20:00
1) manager.Livequote() is the method that a)Adds the Quote into a list that is referenced by Indicator Classes and Strategy Algorithm Class and OrderFill Simulation Class b)Passes the quote into into the StrategyAlgorithm c)Manages order objects in the Strategy to simulate Order Filling / Cancellation / communication latency, etc. — Learning1, Mar 01 '10 at 21:22
2) QuoteLocker is a static object static object QuoteLocker = new object(); — Learning1, Mar 01 '10 at 21:24
3) In terms of behavior, I am expecting each quote as it arrives to be added to each instance of StrategyAssembler and from there added to manager.LiveQuote. From there the strategy works through its algorithm to make the decision to buy / sell /do nothing. Each Assembler instance has only 1 StrategyManager instance and each StrategyManager instance has only 1 Strategy Class that it passes quotes into and manages orders from. The behavior I am seeing has a single quote being added 1 or 2 or 3 or 4 times to a given Assembler instance and not added at all to a different assembler instance — Learning1, Mar 01 '10 at 21:31
Did you find a way to handle the multiple threads in a thread safe manner? — ptn77, Oct 19 '16 at 16:42

score 1 · Answer 1 · answered Mar 01 '10 at 19:41

1

The locking should occur at both the reading and writing to any shared state. Without locks on the read, the code can still read and write concurrently.

You could wrap the read and write locking into the manager.

answered Mar 01 '10 at 19:41

spender

117,338
33
229
351

Thanks for the feedback spender. Are you saying that at every "downstream" point in the code that is reading the quote object I need a lock? (There are a lot of places and objects downstream that uses values from each quote). – Learning1 Mar 01 '10 at 20:02
Wrapping locks into the manager is a dangerous way. Example. If you have a (locked) read, and a (locked) write, this feels safe. But if you do a locked read(), increment, locked write() you'll still have a violation of your critical section. The lock should be around the total usage of the data, resulting in an atomic modification of your data. – Adriaan Mar 01 '10 at 20:53
@spender In this case it's better to copy the quote and avoid locking period... as a matter of fact proper locking would make his strategies run sequentially, it will render the multithreading effort completely useless AND more computationally expensive because of the overhead of locking + context switching. – Kiril Mar 01 '10 at 21:05

score 0 · Answer 2 · answered Mar 01 '10 at 20:12

If:

1) Strategies are invoked via the LiveQuote method and can modify Quote instances.

2) Changes to Quote instances should not be shared among strategies.

You need to create a copy of the provided Quote before calling LiveQuote() and send the copy in to the strategy method rather than the original quote. Depending on other requirements, you may not need any locking at all.

score 0 · Answer 3 · edited May 23 '17 at 10:32

There are two things that are happening in your code:
1. You received a quote from one thread (the producer AKA the market data feed).
2. You send the quote to another thread (the consumer AKA StrategyAssembler).

At this point there is a contention on the quote, in other words the producer thread and each consumer thread (i.e. each instance of a strategy) can modify the quote which you just provide it with. In order for you to remove the contention you must do one of three things:

Synchronize between all of the threads with access to the quote.
OR
Make the quote immutable (and ensure that the producer does not replace it).
OR
Give each consumer a copy of the quote.

For your case I would suggest you take the third option because locking is more expensive than copying a quote (I hope your quotes are not really big)... option two is also good, but your strategy should not modify the quote.

By giving each consumer a copy of the quote you're ensuring that they don't share any data, therefore no other thread will modify the quote and you will eliminate contention. If your strategies are not creating any other threads, then you you're done.

In general you should avoid locking and you should try to minimize data sharing, but if you HAVE TO share data between threads then you should do it properly:
In order for your strategies to synchronize correctly they must synchronize on the same QuoteLocker object, i.e. QuoteLocker must be visible to each thread. Even if you do it properly and you make your strategies synchronize (lock on the QuoteLocker) then you might as well not have threads... you will be running the overhead of context switching + locking and your strategies will be executed sequentially for the same quote.

Update per comments: If you leave the code as is (meaning that you provide a copy of the quote for each thread), then I don't see why your other strategies will not get the quote until the first strategy completes... your first strategy will most likely begin working while the threads for the other strategies are being created. The whole point of making your strategies run in a separate thread is to avoid precisely that problem... you start a new thread so your other strategies are not waiting on each-other to complete.

This part of the code will most likely complete even before all your threads start working...

foreach (StrategyAssembler assembler in StrategyAssembleList.GetStrategies())
 {                  
     BackgroundWorker thread = strategyThreadPool.GetFreeThread();
     if (thread != null)
     {
        thread.DoWork += new DoWorkEventHandler(assembler.NewIncomingQuote);
        Quote copy = CopyTheQuote(quote);// make an exact copy of the quote
        thread.RunWorkerAsync(copy);
     }   
 }

Is your market feed changing the actual quote while you're creating the threads? The market feeds generally provide snapshots, so unless something is changing your quote while you're making the threads, then the design above should be just fine. If there is a problem with the design, then I can give you a producer and multiple consumer design based on a blocking queue which is also very efficient (you can check out this discussion for an idea on how it works and I can tell you how to modify it for your specific example).

Yes, preventing is better than fixing. If your threads can run independently (option 3) you'll have the safest and best performing option. Since you can not guarantee that the first quote is completely handled first, you may need to add a (high resolution) timestamp in your copy. — Adriaan, Mar 01 '10 at 20:59
Thanks for the Feedback Lirik. That all makes a bunch of sense. Option makes the most sense to me for the reasons you mentioned and others also. In terms of making a copy and passing this to each instance of assembler, I think this would be best done with an event so all assemblers are getting their copy of the quote simulataneously and can then process their logic. Otherwise if it is left in the loop, the following strategies won't get the quote until the 1st one completes its logic (about 100ms and quotes can arrive in as little as 50ms). — Learning1, Mar 01 '10 at 21:09
If I put the quote in an event that each assembler subcribes to, then what is the best way of getting each assembler running on a thread so I can take advantage of more cores on the CPU? At the moment, each of these assemblers and the Strategy Algorithm that each has created all run on the Main thread — Learning1, Mar 01 '10 at 21:10
Thanks for the insight Lirik, I'll try this version and see how it workd with the copy of the Quote. In terms of the market changing, the code snippet above is inside the BuildQuote() method. This method is called each time there is a change to BidPrice, BidVolume, AskPrice or AskVolume. Each time it is called, a new Quote is created using static variables that were changed by the Broker Data event. I don't know if a producer and multiple consumer design would be a better way to do this. I'll take a look at the link you posted - thanks! — Learning1, Mar 01 '10 at 21:51
You create a new Quote, but are you replacing the one that's currently being used to initialize the threads? — Kiril, Mar 01 '10 at 21:53

Ensuring Thread Safety

3 Answers3