While writing a custom class derived from FSequenceValidationRule, I came across a crash that I believe to be, at least in part, due to a problematic pattern of using const_cast in the validators’ OnRun implementations. All of the built-in validators do this:
The const_cast seems to be used only because there’s no const API for ISequenceVisitor. I propose that there should be. The problem that this exposes comes from the fact that the SequenceValidator system attempts to run all of these validation rules in parallel - since the Sequence object being inspected by these rules isn’t guaranteed to be const by the implementation code (because of the problematic const_cast), the multithreaded nature of this code is especially prone to race conditions. Of course, respecting the const-ness of the input sequence isn’t going to make this code magically threadsafe by itself… but it would help.
Another issue along the same lines is that FMovieSceneChannel has no const accessor for keys, so any code attempting to inspect channel keys for validation purposes can’t be guaranteed to not modify the owning level sequence.
Thanks for this and yea we agree with your thoughts here.
The real trouble is that the validator runs several rules over the same sequence at once, and that non-const access lands in lazy-caching accessors like UMovieSceneSection::GetChannelProxy() and UMovieSceneTrack::GetEvaluationField() that quietly rebuild cached state on first touch. Two rule threads hitting the same section/track can race on that, which lines up nicely with the crash you saw.
A const ISequenceVisitor API and a const GetKeys would both be good. One thing to flag though: const-correctness by itself won’t make this thread-safe, since some of those caches mutate even on const paths. A real fix also needs to either warm the caches up front on one thread before the parallel run, or make the caches themselves thread-safe.
SequenceValidator is still experimental and our team’s heads-down on other stuff right now, so we can’t really pick this up soon. If you want to take a crack at it, we’d happily look at a GitHub PR. Just know it’s probably a decent chunk of work, since doing it properly means touching the const API and the underlying MovieScene caching/threading, not just the rules themselves.
Yeah, I agree with your notes as well. It does seem like a decent chunk of work, so I don’t think I’ll have time to work on a full-fledged fix for a PR. I’m a fan of the concept of SequenceValidator though, but for now I’m going to bandaid the issue on my end by changing the rules to run sequentially on a single-thread. Thanks for the response.