17
duckWit
2y

8.5 hours spent trying to figure out a race condition ultimately caused by this line.

There really are no words.

Comments
  • 6
    That line looks suspiciously like it has been added to troll future maintainers.
  • 2
    That is just evil
  • 3
    Mind explaining this to me?
  • 4
    It's not a race condition. This looks like a waste of cpu power and needs to be optimized.
    Math.Max of the same value makes no sense to me.
  • 2
    I would hope this gives a compiler warning since it's obviously unintended.
  • 1
    The only reason this would cause a data race - is if the line is executed in parallel threads, writing to the same hashmap.
    The key is the same, but the value is not.
  • 0
    single writer or multiple readers. Where's the issue?
  • 3
    To people asking why this is bad, I believe it’s because they are calculating the max value between the same variables. They intended to have 2 variables but unintentionally duplicated the same var
  • 1
    @IdontHaveAName
    We can see it's bad, are wondering why it would cause a race condition? I suppose I'm mildly surprised that a codebase which evaluates this at all often is in performant condition overall, but that's not the issue.
  • 2
    Hey all. I'll give a little more context.

    (1 of 2)

    This is a specific, single line inside a batch of code that is running in multiple threads. Each thread has a batch of work to do for a particular scriptId, and each one potentially updates this particular dictionary of values. Without explaining all the details, multiple threads sometimes perform work for the same scriptId, but will produce a different value. This dictionary above is only interested in the largest value produced for each scriptId (The line resides within a lock block for thread safety, but I didn't show that part, only the line with the offending bug).
  • 5
    (2 of 2)

    The new value to assign should only be assigned if it is bigger than the current value. Because of the mistake of passing the same parameter twice to Math.Max, each thread would replace the current value for its scriptId with every new potential value, even if it was lower than the previous one.

    So in terms of the race condition, for each scriptId, if thread A assigns a value of 2 and later thread B assigns a value of 4, then there's no problem and everything works. But if thread B beat thread A to it, then thread B's value of 4 would be replaced by thread A's value of 2, and that's the problem.
Add Comment