Fix metrics data race in the Engine test run finalization #1888
Conversation
This should close #1887
| getCachedMetrics: | ||
| for { | ||
| select { | ||
| case sc := <-e.Samples: | ||
| sampleContainers = append(sampleContainers, sc) | ||
| default: | ||
| break getCachedMetrics | ||
| } | ||
| } |
There was a problem hiding this comment.
From the looks of things, this should be called after all the VUs have stopped ... so at least it should not be possible for there to be an endless loop as something keeps adding samples.
Obviously, this won't prevent some extension or badly written internal code to just keep sending metrics without checking the context in a separate goroutine, but I don't think that is such a big problem.
I somewhat would prefer for there to be some timeout to this still, just in case we are wrong or something changes in the future. But I think this can wait
There was a problem hiding this comment.
I am not convinced a timeout is a good idea here... It shouldn't be needed, and we don't have timeouts for other things that process metrics and might be overwhelmed, so this probably shouldn't have one either. That said, I added this issue and mentioned the potential problem in it: #1889
but for now, unless we observe some issues stemming from this, I'm for not touching it 😅
There was a race between these two branches of the
select: https://github.com/loadimpact/k6/blob/f1413e601b021d19f06d4d72ee14714a46344162/core/engine.go#L330-L339I didn't add a test, since apparently the xk6 one I added in #1885 already covers it - yay for integration tests... 🎉 😅 This should close #1887