Skip to content

Commit

Permalink
FAB-9762 Remove VMCResp structure
Browse files Browse the repository at this point in the history 
The response portion of the VMCResp structure is never actually set or
used.  So instead, it only ever wraps an error, or not.  This is much
more complicated than simply returning an error and makes tests and
readability of the code much worse.  This CR simply removes the VMCResp
structure in favor of returning an error.

Change-Id: I12db8441492eb8ad5bf4b52e2eea5f6bbd0ed4fd
Signed-off-by: Jason Yellick <[email protected]>
  • Loading branch information
Jason Yellick committed May 10, 2018
1 parent 1a78254 commit c5fc9a9
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 124 deletions.
8 changes: 4 additions & 4 deletions core/chaincode/chaincode_support_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,22 +226,22 @@ func mockChaincodeStreamGetter(name string) (shim.PeerChaincodeStream, error) {

type mockCCLauncher struct {
execTime *time.Duration
resp container.VMCResp
resp error
retErr error
notfyb bool
}

func (ccl *mockCCLauncher) launch(ctxt context.Context, notfy chan bool) (container.VMCResp, error) {
func (ccl *mockCCLauncher) launch(ctxt context.Context, notfy chan bool) error {
if ccl.execTime != nil {
time.Sleep(*ccl.execTime)
}

//no error on launch, notify
if ccl.resp.Err == nil {
if ccl.resp == nil {
notfy <- ccl.notfyb
}

return ccl.resp, ccl.retErr
return ccl.retErr
}

func setupcc(name string) (*mockpeer.MockCCComm, *mockpeer.MockCCComm) {
Expand Down
22 changes: 4 additions & 18 deletions core/chaincode/container_runtime.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

// Processor processes vm and container requests.
type Processor interface {
Process(ctxt context.Context, vmtype string, req container.VMCReqIntf) (container.VMCResp, error)
Process(ctxt context.Context, vmtype string, req container.VMCReqIntf) error
}

// CertGenerator generates client certificates for chaincode.
Expand Down Expand Up @@ -82,9 +82,8 @@ func (c *ContainerRuntime) Start(ctxt context.Context, cccid *ccprovider.CCConte
}

vmtype := getVMType(cds)
resp, err := c.Processor.Process(ctxt, vmtype, scr)
err = selectError(&resp, err)
if err != nil {

if err := c.Processor.Process(ctxt, vmtype, scr); err != nil {
return errors.WithMessage(err, "error starting container")
}

Expand All @@ -104,9 +103,7 @@ func (c *ContainerRuntime) Stop(ctxt context.Context, cccid *ccprovider.CCContex
Dontremove: false,
}

resp, err := c.Processor.Process(ctxt, getVMType(cds), scr)
err = selectError(&resp, err)
if err != nil {
if err := c.Processor.Process(ctxt, getVMType(cds), scr); err != nil {
return errors.WithMessage(err, "error stopping container")
}

Expand All @@ -120,17 +117,6 @@ func getVMType(cds *pb.ChaincodeDeploymentSpec) string {
return container.DOCKER
}

func selectError(resp *container.VMCResp, err error) error {
switch {
case err != nil:
return err
case resp.Err != nil:
return resp.Err
default:
return nil
}
}

const (
// Mutual TLS auth client key and cert paths in the chaincode container
TLSClientKeyPath string = "/etc/hyperledger/fabric/client.key"
Expand Down
20 changes: 9 additions & 11 deletions core/chaincode/container_runtime_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,18 +209,17 @@ func TestContainerRuntimeStart(t *testing.T) {
func TestContainerRuntimeStartErrors(t *testing.T) {
tests := []struct {
chaincodeType pb.ChaincodeSpec_Type
processResp container.VMCResp
processErr error
errValue string
}{
{pb.ChaincodeSpec_Type(999), container.VMCResp{}, nil, "unknown chaincodeType: 999"},
{pb.ChaincodeSpec_GOLANG, container.VMCResp{}, errors.New("process-failed"), "error starting container: process-failed"},
{pb.ChaincodeSpec_GOLANG, container.VMCResp{Err: errors.New("error-in-response")}, nil, "error starting container: error-in-response"},
{pb.ChaincodeSpec_Type(999), nil, "unknown chaincodeType: 999"},
{pb.ChaincodeSpec_GOLANG, errors.New("process-failed"), "error starting container: process-failed"},
{pb.ChaincodeSpec_GOLANG, errors.New("error-in-response"), "error starting container: error-in-response"},
}

for _, tc := range tests {
fakeProcessor := &mock.Processor{}
fakeProcessor.ProcessReturns(tc.processResp, tc.processErr)
fakeProcessor.ProcessReturns(tc.processErr)

cr := &chaincode.ContainerRuntime{
Processor: fakeProcessor,
Expand Down Expand Up @@ -289,17 +288,16 @@ func TestContainerRuntimeStop(t *testing.T) {

func TestContainerRuntimeStopErrors(t *testing.T) {
tests := []struct {
processResp container.VMCResp
processErr error
errValue string
processErr error
errValue string
}{
{container.VMCResp{}, errors.New("process-failed"), "error stopping container: process-failed"},
{container.VMCResp{Err: errors.New("error-in-response")}, nil, "error stopping container: error-in-response"},
{errors.New("process-failed"), "error stopping container: process-failed"},
{errors.New("error-in-response-is-ignored"), "error stopping container: error-in-response-is-ignored"},
}

for _, tc := range tests {
fakeProcessor := &mock.Processor{}
fakeProcessor.ProcessReturns(tc.processResp, tc.processErr)
fakeProcessor.ProcessReturns(tc.processErr)

cr := &chaincode.ContainerRuntime{
Processor: fakeProcessor,
Expand Down
31 changes: 13 additions & 18 deletions core/chaincode/mock/processor.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 16 additions & 39 deletions core/container/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,10 @@ func (vmc *VMController) unlockContainer(id string) {
//note that we'd stop on the first method on the stack that does not
//take context
type VMCReqIntf interface {
do(ctxt context.Context, v api.VM) VMCResp
do(ctxt context.Context, v api.VM) error
getCCID() ccintf.CCID
}

//VMCResp - response from requests. resp field is a anon interface.
//It can hold any response. err should be tested first
type VMCResp struct {
Err error
Resp interface{}
}

//StartContainerReq - properties for starting a container.
type StartContainerReq struct {
ccintf.CCID
Expand All @@ -128,16 +121,8 @@ type StartContainerReq struct {
FilesToUpload map[string][]byte
}

func (si StartContainerReq) do(ctxt context.Context, v api.VM) VMCResp {
var resp VMCResp

if err := v.Start(ctxt, si.CCID, si.Args, si.Env, si.FilesToUpload, si.Builder); err != nil {
resp = VMCResp{Err: err}
} else {
resp = VMCResp{}
}

return resp
func (si StartContainerReq) do(ctxt context.Context, v api.VM) error {
return v.Start(ctxt, si.CCID, si.Args, si.Env, si.FilesToUpload, si.Builder)
}

func (si StartContainerReq) getCCID() ccintf.CCID {
Expand All @@ -154,16 +139,8 @@ type StopContainerReq struct {
Dontremove bool
}

func (si StopContainerReq) do(ctxt context.Context, v api.VM) VMCResp {
var resp VMCResp

if err := v.Stop(ctxt, si.CCID, si.Timeout, si.Dontkill, si.Dontremove); err != nil {
resp = VMCResp{Err: err}
} else {
resp = VMCResp{}
}

return resp
func (si StopContainerReq) do(ctxt context.Context, v api.VM) error {
return v.Stop(ctxt, si.CCID, si.Timeout, si.Dontkill, si.Dontremove)
}

func (si StopContainerReq) getCCID() ccintf.CCID {
Expand All @@ -178,34 +155,34 @@ func (si StopContainerReq) getCCID() ccintf.CCID {
//context can be cancelled. VMCProcess will try to cancel calling functions if it can
//For instance docker clients api's such as BuildImage are not cancelable.
//In all cases VMCProcess will wait for the called go routine to return
func (vmc *VMController) Process(ctxt context.Context, vmtype string, req VMCReqIntf) (VMCResp, error) {
func (vmc *VMController) Process(ctxt context.Context, vmtype string, req VMCReqIntf) error {
v := vmc.newVM(vmtype)
if v == nil {
return VMCResp{}, fmt.Errorf("Unknown VM type %s", vmtype)
return fmt.Errorf("Unknown VM type %s", vmtype)
}

c := make(chan struct{})
var resp VMCResp
c := make(chan error)
go func() {
defer close(c)

id, err := v.GetVMName(req.getCCID(), nil)
if err != nil {
resp = VMCResp{Err: err}
c <- err
return
}
vmc.lockContainer(id)
resp = req.do(ctxt, v)
err = req.do(ctxt, v)
vmc.unlockContainer(id)
c <- err
}()

select {
case <-c:
return resp, nil
case err := <-c:
return err
case <-ctxt.Done():
//TODO cancel req.do ... (needed) ?
// XXX This logic doesn't make much sense, why return the context error if it's canceled,
// but still wait for the request to complete, and ignore its error
<-c
return VMCResp{}, ctxt.Err()
return ctxt.Err()
}
}

Expand Down
Loading

0 comments on commit c5fc9a9

Please sign in to comment.