From a4332a038ff2aeae587055052a02b34f42eb8f9f Mon Sep 17 00:00:00 2001 From: Yaowen Mei Date: Wed, 13 Mar 2024 04:49:30 +0000 Subject: [PATCH 1/2] Avoid racing in GetFlattenedOutputs() GetFlattenedOutputs() takes the pointer of ec.resPb, and forward it to a long-running grpc call. This will cause racing to happen in re-client unit test framework. See this one line change in re-client that face racing issue: tg/2184373 This change avoid the racing issue by make a deepcopy of resPb before forward it to grpc call. Test: import to re-client and run //internal/pkg/reproxy:reproxy_test with race detection. --- go/pkg/rexec/rexec.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/go/pkg/rexec/rexec.go b/go/pkg/rexec/rexec.go index 91f44ba09..ee5546ceb 100644 --- a/go/pkg/rexec/rexec.go +++ b/go/pkg/rexec/rexec.go @@ -769,10 +769,17 @@ func (ec *Context) DownloadSpecifiedOutputs(outs map[string]*rc.TreeOutput, outD } } -// GetFlattenedOutputs flattens the outputs from the ActionResult of the context and returns -// a map of output paths relative to the working directory and their corresponding TreeOutput +// GetFlattenedOutputs flattens the outputs from the deep copy of an +// ActionResult of the context and returns a map of output paths relative to the +// working directory and their corresponding TreeOutput. func (ec *Context) GetFlattenedOutputs() (map[string]*rc.TreeOutput, error) { - out, err := ec.client.GrpcClient.FlattenActionOutputs(ec.ctx, ec.resPb) + resPb := &repb.ActionResult{} + data, err := prototext.Marshal(ec.resPb) + prototext.Unmarshal(data, resPb) + if err != nil { + return nil, fmt.Errorf("failed to marshal ec.resPb %v: %v", ec.resPb, err) + } + out, err := ec.client.GrpcClient.FlattenActionOutputs(ec.ctx, resPb) if err != nil { return nil, fmt.Errorf("Failed to flatten outputs: %v", err) } From 45bf0782667f5ccbbe8941e2c75a24f81a6fa182 Mon Sep 17 00:00:00 2001 From: Yaowen Mei Date: Wed, 13 Mar 2024 04:57:53 +0000 Subject: [PATCH 2/2] Avoid racing in GetFlattenedOutputs() GetFlattenedOutputs() takes the pointer of ec.resPb, and forward it to a long-running grpc call. This will cause racing to happen in re-client unit test framework. See this one line change in re-client that face racing issue: tg/2184373 This change avoid the racing issue by make a deepcopy of resPb before forward it to grpc call. Test: import to re-client and run //internal/pkg/reproxy:reproxy_test with race detection. --- go/pkg/rexec/rexec.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/go/pkg/rexec/rexec.go b/go/pkg/rexec/rexec.go index 91f44ba09..b40dc0a25 100644 --- a/go/pkg/rexec/rexec.go +++ b/go/pkg/rexec/rexec.go @@ -769,10 +769,20 @@ func (ec *Context) DownloadSpecifiedOutputs(outs map[string]*rc.TreeOutput, outD } } -// GetFlattenedOutputs flattens the outputs from the ActionResult of the context and returns -// a map of output paths relative to the working directory and their corresponding TreeOutput +// GetFlattenedOutputs flattens the outputs from the deep copy of an +// ActionResult of the context and returns a map of output paths relative to the +// working directory and their corresponding TreeOutput. func (ec *Context) GetFlattenedOutputs() (map[string]*rc.TreeOutput, error) { - out, err := ec.client.GrpcClient.FlattenActionOutputs(ec.ctx, ec.resPb) + resPb := &repb.ActionResult{} + data, err := prototext.Marshal(ec.resPb) + if err != nil { + return nil, fmt.Errorf("failed to marshal ec.resPb %v: %v", ec.resPb, err) + } + err = prototext.Unmarshal(data, resPb) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal ec.resPb %v: %v", ec.resPb, err) + } + out, err := ec.client.GrpcClient.FlattenActionOutputs(ec.ctx, resPb) if err != nil { return nil, fmt.Errorf("Failed to flatten outputs: %v", err) }