Receiver: Fix data race in writer (label set copying) #5492
Conversation
matej-g
force-pushed
the
fix-data-race-receive-writer-labels
branch
from
8cc31b1 to
0743b8d
Compare
|
I also added benchmark for realloc version with |
Signed-off-by: Matej Gera <[email protected]>
Signed-off-by: Matej Gera <[email protected]>
matej-g
force-pushed
the
fix-data-race-receive-writer-labels
branch
from
0743b8d to
a78cf81
Compare
There was a problem hiding this comment.
Hm, where we modify them? Maybe instead we could stop modifying in place?
My worry is that the same hidden modification might be done if ref =! 0 - we have no mean to verify it. And IF that happens I really we don't copy all labels - I don't think all of them are changed.
| lset = labelpb.ZLabelsToPromLabels(t.Labels) | ||
| // | ||
| // Nevertheless, we have to make a deep copy, to prevent manipulating the request | ||
| // labels (these are re-used among different requests and could trigger data race). |
There was a problem hiding this comment.
I put it in the description above:
Based on the logs, my assumption is that in the handler, we are re-using the *prompb.WriteRequest in the fanoutForward method - once for the local node Write and then for the remote forward in RemoteWrite. It seems that doing some reallocation shuffling with the labels can interfere with gRPC message marshalling, as it seem to be re-using the same underlying memory.
I also pasted the output when data race is detected.
|
The results seem to show quite a big difference (from 1.535 ns/op to 61866060 ns/op, which means 61ms), but only reading code above we can see that's the use case for I suspect the performance difference for scenarios like 50 labels or so would be unnoticeable. There might still be interesting change in a high throughput scenario. |
|
What if we protect the realloc for local write with a mutex and check it before the fanout gRPC writes? 🤔 |
|
I am not fully bought on this honestly, need to look deeper 🙈 |
|
Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days. |
|
Closing for now as promised, let us know if you need this to be reopened! 🤗 |
Signed-off-by: Matej Gera [email protected]
Changes
This is continuation of fixes detected during work on #4664.
We appear to be hitting a data race due to some concurrent read / write in write request's labels. See output:
Based on the logs, my assumption is that in the handler, we are re-using the
*prompb.WriteRequestin thefanoutForwardmethod - once for the local nodeWriteand then for the remote forward inRemoteWrite. It seems that doing some reallocation shuffling with the labels can interfere with gRPC message marshalling, as it seem to be re-using the same underlying memory.The suggestion here is to make a short-lived separate copy of the label set (if ref is not available) before proceeding with the write on the local node. This should ensure we are no longer writing to the same object as we're reading from during marshalling.
I'm not 100% not sure of performance implications though, seeing this is the main write path for receive.
Verification
Rate the E2E reciever tests on top of #5066 with no data race being detected anymore.