Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing trello #1874 bug: Algo dies with System.InvalidOperationExcept… #730

Merged

Conversation

quant1729
Copy link
Contributor

…ion: Collection was modified; enumeration operation may not execute.

…rationException: Collection was modified; enumeration operation may not execute.
@jaredbroad
Copy link
Member

Thanks @quant1729, did you identify who the two threads were? Could there be any state issues by the two threads accessing it at once?

@mchandschuh
Copy link
Contributor

@jaredbroad - my thoughts exactly. This 'fix' may just be hiding a bug, ideally we would synchronize the two threads accessing this data, otherwise it will be non deterministic

@quant1729
Copy link
Contributor Author

in both modes 3 - 4 threads touch cashbook throughout its lifetime. Measured with my counter in class methods. Example threads in backtest: brokerage transaction handler, algorithm manager, main engine. Example in live mode: IB brokerage, algorithm manager, main engine.

@mchandschuh Can you please elaborate on this with code details? As the author of the code I believe you know more on the ideas behind cashbook in MT environment.

@mchandschuh
Copy link
Contributor

The concern is not specific to the cash book, but rather any data being accessed by multiple threads. The exception is caused because one thread is reading the data while another is modifying it, so there exists the potential for a bug. Is it safe for a thread to 'miss' a value? The only way to make that determination is to figure out who is reading and writing the data and do they need to be synchronized to ensure data is not dropped from the read operation

@jaredbroad
Copy link
Member

TODO on a future date: review all the live trading cross thread calls and use locks to sync. This PR points to larger work needing done in the asynchronous calls in live (brokerage events, result handler etc)

For now PR approved as most common cause is the result handler displaying the portfolio values; which isn't critical and is recalculated a second later; so safe enough to merge in.

@quant1729 please log when these events occur so the information/request isn't lost and if it causes follow on errors we'll know where they come from.

@quant1729
Copy link
Contributor Author

Added todo item to trello (on me). Adding logging now.

@jaredbroad jaredbroad merged commit ae9d7f4 into QuantConnect:master Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants