Thread-safety: Change property of items in arraylist versus removingitems from the arraylist

[Curious]

I have an arraylist, OrderList, in which each item in the arraylist is
an Order object.

At the beginning of my program, I read from an input file to create
the OrderList. Then I launch several threads for different operations
related to OrderList.

Thread #1: "OnOrderFilled" method is REMOVING an order from the
arraylist once the order is filled.
Thread #1: "OnOrderCancelled" method is changing the property of an
order once the order is cancelled.

Thread #2: "ActivateOrder" method loops through each order in
OrderList and sends out the orders.

Here's the problem - When "ActivatOrder" sends out orders (please note
that it takes time to send out orders), some orders may be filled at
the same time; so the "OnOrderFilled" method on Thread #1 is fired
which REMOVES the orders from OrderList that are filled, and this
affects "ActivateOrder" method that loops through OrderList. I get
"Collection was modified" exception.

To get this fixed, I was thinking of instead of REMOVING order from
OrderList that causes "Collection was modified" error, I should add a
property to each Order called "IsFilled" so that I CHANGE "IsFilled"
to true when an order is filled.

My question is -- Will changing the property in "OnOrderFilled" method
cause any thread-safety issue for "ActivateOrder" method? Thanks!

 

[michel.desangles]

If I'm not mistaken, you should never modify a collection which is
being iterated. To answer your question more precisely: if the
property OnOrderFilled doesn't participate in the hashcode calculation
of your Order object, then changing it won't raise the exception, but
it still won't be a good idea.

You could lock the collection, but if ActivateOrders takes some time
as you indicate, locking it would probably hang various parts of your
program while the orders are processed.

I think you'd be better off iterating on a copy of the collection,
which would be local to your second thread. I would probably go for a
deep copy, in order to avoid concurrent access from the first thread.
If your collection is not too large, copying it shouldn't be too
expensive.

Michel

 

[Göran Andersson]

I have an arraylist, OrderList, in which each item in the arraylist is
an Order object.

At the beginning of my program, I read from an input file to create
the OrderList. Then I launch several threads for different operations
related to OrderList.

Thread #1: "OnOrderFilled" method is REMOVING an order from the
arraylist once the order is filled.
Thread #1: "OnOrderCancelled" method is changing the property of an
order once the order is cancelled.

Thread #2: "ActivateOrder" method loops through each order in
OrderList and sends out the orders.

Here's the problem - When "ActivatOrder" sends out orders (please note
that it takes time to send out orders), some orders may be filled at
the same time; so the "OnOrderFilled" method on Thread #1 is fired
which REMOVES the orders from OrderList that are filled, and this
affects "ActivateOrder" method that loops through OrderList. I get
"Collection was modified" exception.

To get this fixed, I was thinking of instead of REMOVING order from
OrderList that causes "Collection was modified" error, I should add a
property to each Order called "IsFilled" so that I CHANGE "IsFilled"
to true when an order is filled.

My question is -- Will changing the property in "OnOrderFilled" method
cause any thread-safety issue for "ActivateOrder" method? Thanks!

It's possible, but you would have to use the same lock on all members of
the object that is used by the different threads.

You want to prevent the properties of the object to be change while the
order is being processed by the ActivateOrder method, and you want to
prevent the ActivateOrder method to send the order while the property is
being changed.

If the code for sending the order is outside the class, you would have
to add another syncronised property IsSending, that is set to true when
the process starts and set to false when it's done. It should not be
possible to change any properties in the object while IsSending is true.


Add a private variable in the object to use for synchronising:

private object _sync = new object();

This way the class will handle all it's locking itself, so there is
minimal risk of creating deadlocks because code ouside the class is
locking on the same reference.