Conversation
This ensures that events will be delivered in a more predictable order which mitigates race conditions, and avoids thread/memory leaks in ScoreBoardEventProviderManager. Leave DefaultScoreBoardModel as-is for now.
This is a wrapper which serves events asynchronously via a thread. This is used for places that are talking to the outside world so that they don't inadvertantly hold up the entire system if an external dependency is slow.
Make event delivery within DefaultScoreBoardModel async, to avoid deadlocks.
|
Did you actually encounter deadlocks in DefaultScoreBoardModel or are you just being cautious? |
|
I encountered deadlocks immediately in actual usage (but not in unittests). Twitter/XML/WS is me being cautious. Twitter/XML I don't know enough about how they work, WS can run out of kernel TCP buffer space. |
|
Without knowing the specific cause of the deadlocks, I'm somewhat worried that DefaultTeamModel or DefaultClockModel could also be vulnerable to them. Both classes also have functions that are called from the frontend and trigger events which are in turn sent back to the frontend. (And having two threads do this with an unlucky ordering of locks sounds like a candidate for the source of the deadlocks.) |
|
Neither DefaultClockModel nor DefaultTeamModel have a listener. DefaultTeamModel just passes events up to the DefaultScoreBoardModel, as it's the clearing house for events. I believe the deadlocks are due to the various ad-hoc locking patterns in use, and running all the events async as I'm doing in DefaultScoreBoardModel should prevent any issues. |
frank-weinberg
left a comment
frank-weinberg
left a comment
There was a problem hiding this comment.
Aside from the concerns regarding deadlocks, I have no objections to the change. And while those concerns are sufficient for me to not merge the PR, I don't object, if you want to do it yourself.
|
Thanks! I don't think there's any more risk than what we have today, in fact I'd expect it to be a bit less. We'll see at British Champs this weekend anyway. |
In order to avoid potential for deadlocks, cover all the core logic in Default*Model with a single lock. This could make reaction times a bit slower, if multiple clients send state updates at the same time but as long as all updates are handled quickly, this should be negligible. (We should, however, not start to compute complex stats under the lock.) The only remaining calls from code under the lock to code (potentially) under different locks are to the baseLock in Ruleset.java, which in turn does not reach other locks, and through the event interface. Since all event listeners not covered by the coreLock are async and thus the actual handling of the event will not be done while holding the coreLock, we can conclude that this change and rollerderby#149 together do not introduce any potential deadlocks.
In order to avoid potential for deadlocks, cover all the core logic in Default*Model with a single lock. This could make reaction times a bit slower, if multiple clients send state updates at the same time but as long as all updates are handled quickly, this should be negligible. (We should, however, not start to compute complex stats under the lock.) The only remaining calls from code under the lock to code (potentially) under different locks are to the baseLock in Ruleset.java, which in turn does not reach other locks, and through the event interface. Since all event listeners not covered by the coreLock are async and thus the actual handling of the event will not be done while holding the coreLock, we can conclude that this change and #149 together do not introduce any potential deadlocks.
In order to avoid potential for deadlocks, cover all the core logic in Default*Model with a single lock. This could make reaction times a bit slower, if multiple clients send state updates at the same time but as long as all updates are handled quickly, this should be negligible. (We should, however, not start to compute complex stats under the lock.) The only remaining calls from code under the lock to code (potentially) under different locks are to the baseLock in Ruleset.java, which in turn does not reach other locks, and through the event interface. Since all event listeners not covered by the coreLock are async and thus the actual handling of the event will not be done while holding the coreLock, we can conclude that this change and rollerderby#149 together do not introduce any potential deadlocks.
2 participants
This switches event delivery to be largely synchronous, and also in-order. Only one event is executed at a time.
Async is kept for the scoreboard itself, to avoid deadlocks, and when talking to systems like Twitter/XML/WS who may stall.
This is simpler, more predictable, and also happens to halve the length of the unittests.
@frank-weinberg